-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #13347 #13562 #14285: Introduce new attribute proto_size_in_bytes for Exploration models #13391
Conversation
Hi @anandwana001, can you complete the following:
|
Hi, @anandwana001, this pull request does not have a "CHANGELOG: ..." label as mentioned in the PR pointers. Assigning @anandwana001 to add the required label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks! |
Hi @anandwana001, when creating WIP/Draft PRs, ensure that your commit messages are prefixed with [ci skip] or [skip ci] to prevent CI checks from running. You can learn more about it here. |
1 similar comment
Hi @anandwana001, when creating WIP/Draft PRs, ensure that your commit messages are prefixed with [ci skip] or [skip ci] to prevent CI checks from running. You can learn more about it here. |
Hi @DubeySandeep, this PR adds a new code owner, @anandwana001 to the following files /core/domain/exploration.proto, /core/domain/exploration_pb2.py. Please make sure the changes are verified by the previous codeowner(s) of the file. Thanks! |
Hi @DubeySandeep, this PR adds a new code owner, @anandwana001 to the following files /core/domain/exploration.proto, /core/domain/exploration_pb2.py. Please make sure the changes are verified by the previous codeowner(s) of the file. Thanks! |
Hi @DubeySandeep, this PR adds a new code owner, @anandwana001 to the following files /core/domain/exploration.proto, /core/domain/exploration_pb2.py. Please make sure the changes are verified by the previous codeowner(s) of the file. Thanks! |
1 similar comment
Hi @DubeySandeep, this PR adds a new code owner, @anandwana001 to the following files /core/domain/exploration.proto, /core/domain/exploration_pb2.py. Please make sure the changes are verified by the previous codeowner(s) of the file. Thanks! |
Hi @anandwana001, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks! |
Hi @DubeySandeep, this PR adds a new code owner, @anandwana001 to the following files /core/domain/exploration.proto, /core/domain/exploration_pb2.py. Please make sure the changes are verified by the previous codeowner(s) of the file. Thanks! |
Hi @anandwana001. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
Hi @anandwana001, when creating WIP/Draft PRs, ensure that your commit messages are prefixed with [ci skip] or [skip ci] to prevent CI checks from running. You can learn more about it here. |
1 similar comment
Hi @anandwana001, when creating WIP/Draft PRs, ensure that your commit messages are prefixed with [ci skip] or [skip ci] to prevent CI checks from running. You can learn more about it here. |
Hi @anandwana001, there is a new change in develop which needs to be in your PR. Please update your branch with the latest changes in develop. For instructions, refer to this link. Thanks! |
Hi @anandwana001, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
@vojtechjelinek I had added this new job as per our discussion.
|
Hi @anandwana001. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
For the missing field instead of returning |
Hi @anandwana001, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks! |
Done, please let me know if it works now? |
Hi @anandwana001, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THanks! Took a pass.
hasAttributes = True | ||
else: | ||
hasAttributes = False | ||
except Exception as e: | ||
logging.exception(e) | ||
return result.Err((exp_id, e)) | ||
|
||
if hasAttributes: | ||
return result.Ok((exp_id, hasAttributes)) | ||
else: | ||
return result.Err((exp_id, e)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasAttributes = True | |
else: | |
hasAttributes = False | |
except Exception as e: | |
logging.exception(e) | |
return result.Err((exp_id, e)) | |
if hasAttributes: | |
return result.Ok((exp_id, hasAttributes)) | |
else: | |
return result.Err((exp_id, e)) | |
return result.Ok((exp_id, hasAttributes)) | |
else: | |
return result.Err((exp_id, e)) | |
except Exception as e: | |
logging.exception(e) | |
return result.Err((exp_id, e)) |
{ | ||
'exploration_boolean_list': exploration_boolean_list | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you pass just exploration_boolean_list
?
|
||
return ( | ||
( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exploration_objects_list_job_run_results = ( | ||
exploration_objects_list | ||
| 'Transform exploration objects into job run results' >> ( | ||
job_result_transforms.CountObjectsToJobRunResult( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don'ŧ you also use ResultsToJobRunResults
here (I know that you may need to change some stuff above)?
Unassigning @vojtechjelinek since the review is done. |
Hi @anandwana001, there is a new change in develop which needs to be in your PR. Please update your branch with the latest changes in develop. For instructions, refer to this link. Thanks! |
Hi @anandwana001, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
Overview
proto_size_in_bytes
in the exploration model. This attribute will contain the proto size of the exploration.[oppia-proto-api
](https://github.com/oppia/oppia-proto-api/tree/introduce-proto-api-v1) repository.to_proto
for the classes which are required to fill the proto fields and other respective functions:core/tests/data/oppia-ThetitleforZIPdownloadhandlertest!-v2-gold.zip
- This zip file is updated by adding this new attributeproto_size_in_bytes
. This can be reviewed by locally unzipping and checking if the attribute is correct or not.scripts/install_third_party_libs.py
.TODO
Essential Checklist
Proof that changes are correct
PR Pointers