-
-
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 #14698: Add ability to generate X sample translation opportunities on local dev server #14889
Conversation
…also make opportunity list hidden when LoadingMoreData
Hi @jimbyo, can you complete the following:
|
Hi @nithusha21, @DubeySandeep -- could one of you please add the appropriate changelog label to this pull request? Thanks! |
Closing for now because of bugs when generating opportunities multiple times |
Hi @jimbyo, can you complete the following:
|
Hi @jimbyo, it looks like some changes were requested on this pull request by @sagangwee. PTAL. Thanks! |
Hi @jimbyo, 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. |
Hi @jimbyo, 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.
Took another pass! PTAL! @sagangwee
core/controllers/admin.py
Outdated
elif action == 'generate_sample_opportunities': | ||
num_sample_ops = self.normalized_payload.get( | ||
'num_sample_ops') | ||
num_sample_interactions = self.normalized_payload.get( |
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.
done
core/controllers/admin.py
Outdated
Args: | ||
num_sample_ops: int. Count of sample opportunities to | ||
be generated. | ||
num_sample_interactions: int. Count of sample |
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.
done
core/controllers/admin.py
Outdated
be generated. | ||
num_sample_interactions: int. Count of sample | ||
interactions to be generated. | ||
should_submit_suggestions: boolean. Boolean to track |
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.
done
core/controllers/admin.py
Outdated
should_submit_suggestions: boolean. Boolean to track | ||
whether suggestions should be submitted. | ||
""" | ||
topic_id_1 = topic_fetchers.get_new_topic_id() |
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.
changed to topic id and also changed topic_1 to topic
core/controllers/admin.py
Outdated
story_id, exp_ids_in_story) | ||
|
||
if should_submit_suggestions: | ||
for i in range(num_sample_ops): |
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.
done. Changed i to opportunity_num and j to interaction num
core/controllers/admin.py
Outdated
new_exploration_id, title=title, category=category, | ||
objective='Dummy Objective') | ||
|
||
for interaction_num in range(num_sample_interactions): |
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.
created helper
core/controllers/admin.py
Outdated
objective='Dummy Objective') | ||
|
||
for interaction_num in range(num_sample_interactions): | ||
if interaction_num == num_sample_interactions - 1: |
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.
Done!
Also quick question. When should we have helper functions inside a function and outside a function
core/controllers/admin.py
Outdated
len_story = len(story_node_dicts) | ||
for i, story_node_dict in enumerate(story_node_dicts): | ||
self._generate_sample_story_nodes( | ||
story, len_story, i + 1, **story_node_dict) |
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.
story node ids start at one not zero, when they start at zero the code fails. I have changed enumerate(story_node_dicts)
to enumerate(story_node_dicts, 1)
to make this more clear than the previous i + 1
""" | ||
|
||
story.add_node( | ||
'%s%d' % (story_domain.NODE_ID_PREFIX, node_id), |
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.
Sorry what are you talking being duplicated? This portion?
'%s%d' % (story_domain.NODE_ID_PREFIX, node_id)
Hi @jimbyo, 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.
@jimbyo Sorry for the delay! Took a pass. Thanks.
Track whether suggestions should be submitted. | ||
|
||
Raises: | ||
Exception. Environment is not DEVMODE. |
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.
DEV_MODE*
num_sample_interactions_per_opportunity: int. Number of sample | ||
interactions to generate per opportunity. | ||
should_submit_suggestions: boolean. | ||
Track whether suggestions should be submitted. |
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.
Cam omit "Track"
num_sample_interactions_per_opportunity, | ||
should_submit_suggestions) | ||
|
||
def _generate_sample_opportunities_without_existing_topic( |
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.
Generally order functions in the order that they're used, so this should be below _generate_sample_opportunities_with_existing_topic
.
self, num_sample_ops, | ||
num_sample_interactions_per_opportunity, | ||
should_submit_suggestions): | ||
"""Generates a topic and story and creates |
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.
Bring some of the sentence to this line up to the line limit?
interactions to generate per opportunity. | ||
should_submit_suggestions: boolean. | ||
Track whether suggestions should be submitted. | ||
story_url_fragment: stringType. Story fragment of existing story. |
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.
*string ?
Also, what does "Story fragment of existing story" mean? The description should something like "The url fragment of the story." right? And if the story_url_fragment
is always set to 'sample-story-title', then it doesn't seem like this arg is necessary.
have been generated. | ||
num_sample_interactions_per_opportunity: int. Number of sample | ||
interactions that have been generated. | ||
exp_ids_in_story: list. List of the exploration ids of explorations |
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.
Nit: Since we specify the "list" type already, we can just omit the "List of" part of the description. Here and below.
title = random.choice(possible_titles) + ' ' + str( | ||
opportunity_num + num_chapters) |
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.
Prefer using string interpolation instead of concatenation here (easier to read).
|
||
for interaction_num in range(num_sample_interactions_per_opportunity): | ||
if is_last_interaction(): | ||
dest = 'End' |
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.
Nit: destination and current_state seem short enough to not abbreviate.
"""Populates a sample interaction | ||
and links to a destination interaction. |
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.
Fix line limit here as well.
""" | ||
|
||
story.add_node( | ||
'%s%d' % (story_domain.NODE_ID_PREFIX, node_id), |
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.
Yeah. It would read better if we stored that portion to a variable, e.g. node_name
.
Unassigning @sagangwee since the review is done. |
Hi @jimbyo, it looks like some changes were requested on this pull request by @sagangwee. PTAL. Thanks! |
Hi @jimbyo, 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 @jimbyo, 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
Essential Checklist
Proof that changes are correct
Video
Screen.Recording.2022-02-15.at.4.22.51.PM.mov
Video 2
Screen.Recording.2022-02-23.at.6.00.45.PM.mov