-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat: add minimal atlas
step to the build FC-0012
#41
Conversation
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.
I like your approach of testing Palm on a smaller Tutor plugin. And I am also looking forward to ditching my very custom solution based on openedx-i18n. Let's try to get Atlas working on this repo before we attempt to tackle the openedx Dockerfile.
I'm curious, did you manage to build the discovery image with this PR? If yes, did you run your own fork of course-discovery? I did not test your changes but I suspect that I would not be able to build the Docker image without transifex credentials.
# Collect static assets | ||
# Update i18n and collect static assets | ||
{% if DISCOVERY_ATLAS_PULL %} | ||
RUN make OPENEDX_ATLAS_PULL=yes ATLAS_PULL_LANGS="{{ DISCOVERY_ATLAS_PULL_LANGS }}" pull_translations |
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.
- The
DISCOVERY_ATLAS_PULL_LANGS
setting is undefined. How can we make Atlas automatically (or at least: by default) download strings from all "sufficiently translated" languages? As a rule of thumb, we like to include in Tutor all languages with >50% reviewed strings. - In Palm, the
make pull_translations
command is going to runtx
, and notatlas
. Actually, neither does the master branch run Atlas. - Also, how are we going to pull translations from Transifex (I assume?) if there is no transifexrc file in the Docker image?
- Actually, do we really have to run a
make
command? Can we just runatlas
directly to remove one level of indirection? - Once we have a basic version working, we should try to move the atlas step to a separate Docker layer.
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.
The DISCOVERY_ATLAS_PULL_LANGS setting is undefined. How can we make Atlas automatically (or at least: by default) download strings from all "sufficiently translated" languages? As a rule of thumb, we like to include in Tutor all languages with >50% reviewed strings.
True. I'll fix this.
In Palm, the make pull_translations command is going to run tx, and not atlas. Actually, neither does the master branch run Atlas.
This is work in progress and we're adding it to tutor at the moment. We will not merge unless everything is ready.
Also, how are we going to pull translations from Transifex (I assume?) if there is no transifexrc file in the Docker image?
Transifex won't be used directly anymore in Open edX. At least this is the direction we're going to according to OEP-58.
Actually, do we really have to run a make command? Can we just run atlas directly to remove one level of indirection?
No really. Except for MFEs, in which the make
command is needed since it depends on the context: https://github.com/openedx/frontend-app-learner-dashboard/blob/master/Makefile#L63-L71
Once we have a basic version working, we should try to move the atlas step to a separate Docker layer.
I'd like to learn more what do you mean by this @regisb?
Thanks for the notes, obviously this is an experiment so your feedback is very helpful!
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.
I'd like to learn more what do you mean by this @regisb?
It's a bit difficult to explain in writing. It has to do with not repeating the translation pull step unless necessary. The goal being to make it as fast as possible to re-build Docker images. It's OK, we can figure this out later.
I converted this PR to a draft, let me know when it's ready for another round of review. |
1b35356
to
1fdce16
Compare
@regisb I've addressed some of your notes. I think we still need some way to pass options for example:
Another feature is that Atlas and Open edX TranslationsPerhaps, it's good to share a recap on what happened on Atlas. Part of OEP-58, we've created two new additions to the Open edX Platform https://github.com/openedx/openedx-translations and https://github.com/openedx/openedx-atlas .
Testing
File treeThe file tree seems about right. It removed the old files and add fresh ones from the repository (as opposed to Transifex).
|
I think we should wait until we implement openedx/openedx-atlas#34 and use pip |
@regisb and @brian-smith-tcril would you mind taking a look at this PR? It should be ready again for review. |
@regisb I've updated the PR except for the Anyway, it's not a blocker for this PR so we can remove it until we figure out another alternative.
|
5929fef
to
f98d3bb
Compare
@regisb what do you think should happen if a Currently the docker build fails, which I think isn't appropriate. I'm thinking we should add validation to the https://github.com/openedx/openedx-translations so this won't happen inside Tutor, correct? |
@OmarIthawi I made an issue on the |
Thanks Brian and Regis. I'll fix the openedx-translations repo first then I'll continue with this PR. |
@regisb @brian-smith-tcril would mind taking another look? This took a lot of experimentation and review effort, but I hope it's worth it. I've also made sure that |
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.
In addition to my comment, can you please add a changelog entry with scriv create
?
openedx-atlas==0.4.4 | ||
|
||
# Update i18n: the equivalent of "make pull_translations" | ||
RUN find course_discovery/conf/locale -type d -mindepth 1 -maxdepth 1 -exec rm -rf {} + # Remove stale translation files |
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.
This causes all translation files currently in course_discovery/conf/locale to be deleted. Not all translation files will be replaced by atlas pull
. This means that some languages will no longer be available to existing users, which is not acceptable.
Is there an alternate solution? Can we just skip the deletion step?
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.
This causes all translation files currently in course_discovery/conf/locale to be deleted. Not all translation files will be replaced by
atlas pull
. This means that some languages will no longer be available to existing users, which is not acceptable.
This ideally shouldn't be a concern as long as both projects translations have the same completeness. Which is still work in progress.
Once OEP-58 is fully implemented, repos will have no translations committed. Again, removing the deletion step is future-proof, so it should be fine.
Is there an alternate solution? Can we just skip the deletion step?
I'll see if skipping the deletion step works.
I'll have to share few minor concerns that this PR should balance:
-
What if some translations are broken in the original repo? Then
atlas pull
won't fix those errors. What do you think? -
In MFEs, it's preferable to have only the used languages to reduce the JavaScript bundle because translations are compiled and bundled into the code. But this isn't an MFE.
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.
This ideally shouldn't be a concern as long as both projects translations have the same completeness. Which is still work in progress.
OK. So this PR is not 100% ready to be merged, right?
What if some translations are broken in the original repo? Then atlas pull won't fix those errors. What do you think?
I'm fine with existing bugs, I just don't want to create new ones. It's not atlas' job to fix existing translations issues.
In MFEs, it's preferable to have only the used languages to reduce the JavaScript bundle because translations are compiled and bundled into the code. But this isn't an MFE.
That's a good point. But then again, it's not like MFE bundles are suddenly going to get very big. They already contain all the translation strings, right? Plus, it seems to me that translations strings take little size compared to vendor libraries.
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.
OK. So this PR is not 100% ready to be merged, right?
Yes, I somewhat missed this big issue. My bad for removing the flag which I orgininally added in thi
We want to ship it behind a feature flag like all the other repos, then we'll flip the switch once the Transifex project is fully migrated.
I need to get back the "OPENEDX_ATLAS_PULL" flag which has been the standard in all repositories such as:
- Learning MFE: https://github.com/openedx/frontend-app-learning/blob/32bd3190a6f26bc16472853de11a06f462205d82/Makefile#L46-L63
- Course Discovery: https://github.com/Zeit-Labs/course-discovery/blob/23406673c226f042565144949b585d7e37ae9ffe/Makefile#L106-L119
That's a good point. But then again, it's not like MFE bundles are suddenly going to get very big. They already contain all the translation strings, right? Plus, it seems to me that translations strings take little size compared to vendor libraries.
Correct. This is still an open discussion in the Transifex Working Group. But yeah, it shouldn't be a concern in this context.
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 about the review delay)
Honestly I'm not a big fan of the feature flag. Managing feature flags across multiple repositories is a pain which I'd rather avoid. Either the feature is ready for launch, in which case we don't need the feature flag in Tutor, or it is not, and in that case atlas pull
should be implemented with a plugin. (Yes, that would be a plugin for a plugin...)
If the latter, then all your changes can be added to the discovery Dockerfile with a single extra {{ patch("discovery-dockerfile") }}
statement (we would just have to think about that patch name and add it to this repo).
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
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.
@regisb I just saw your comment 😿
It's ready'ish, so no it's not ready.
It'll only be considered ready once someone else approves it to be and uses it on their stacks. So it's puts us in a chicken-and-egg issue.
I'll check with the team on the plugin decision and get back to you. It seems to be a good solution, except for the timeline potential delays.
I'll be somewhat a pivot which needs more exploration (and more Tutor learning for sure) from my side.
@brian-smith-tcril from timeline perspective, what do you think about creating a plugin for atlas on discovery, or for Tutor in general?
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.
I'm eager to see Atlas succeed and don't want to cause extra delays. If it's a chicken and egg thing, and building a plugin would further delay the project, then we can help resolve the situation by hiding the feature behind an undocumented feature flag in the tutor-discovery plugin, just like you suggested. We should make it clear that it's a temporary, unsupported feature flag, and the flag should not be present in other core plugins.
Thus I no longer have any objection to your PR and I'd say I'm ready to merge it when you are.
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.
To be clear, making it a plugin isn't a bad choice, in fact it's pretty good one. However, it's bit more costly than direct pull requests, so I'd like to hear from @brian-smith-tcril.
I imagine if we have an tutor-atlas
plugin which adds Tutor patches to all other plugins would be a nice way to make atlas optional.
the feature behind an undocumented feature flag in the tutor-discovery plugin, just like you suggested
Great, thanks for being open for the lesser option for the sake of timeline. Today, we'll know which way to go.
Options to implement Atlas
Option 1: enable always
-
Perfect the translations workflow
- Sync all languages to the repo
- Ensure no invalid translations being committed to the repository
-
Enable atlas by default without a feature flag on Tutor nightly core and plugins
This option makes it very slow to get feedback and risks the timeline push.
Option 2: temporary feature flag first
- Enable atlas behind an undocumented temp. feature flag on Tutor nightly core and plugins
- Perfect the translations worflow in parallel
- Enable atlas by default without a feature flag (Same as Option 1)
With this option, others can use atlas faster.
Option 3: plugin first
- Create an tutor-atlas plugin to use atlas on Tutor core and other plugins
- Perfect the translations worflow in parallel
- Once completed, tutor-atlas will be moved to core (Same as Option 1)
This option makes it is faster than Option 1 in terms of feedback, but also slower than Option 2 and could impact the timeline negatively.
@regisb the pull request is ready again for review. |
atlas
step to the buildatlas
step to the build
@regisb a gentile reminder about this pull request :) |
pulling in @Faraz32123 who is the new maintainer for this plugin. |
@Faraz32123 would you mind checking this pull request? |
@OmarIthawi did you see my last comment above? |
@regisb Let's please continue with the feature flag the three of us thinks it's the way to go. I don't fully understand Tutor branching strategy, so should I change the base branch from |
Do you want the feature in the Palm release? If yes, then keep the base branch as master, and it will also be included in nightly. If the feature should only target the upstream master branch of the course-discovery repo, aim for the nightly branch here. For more information: https://docs.tutor.overhang.io/tutorials/nightly.html |
@regisb for the discovery, I think it should go to All other |
To be clear: if atlas goes to the plugin master branch, the change will also be included in Quince. We should push to nightly only if the change does not work in Palm. Does that make sense? |
Yes, makes sense :) I don't want |
This contribution is part of the [FC-0012 project](https://openedx.atlassian.net/l/cp/XGS0iCcQ) which is sparked by the [Translation Infrastructure update OEP-58](https://open-edx-proposals.readthedocs.io/en/latest/architectural-decisions/oep-0058-arch-translations-management.html#specification).
@regisb @Faraz32123 I've rebased over Sample output from the build logs
I think this pull request is ready to be merged. BTW, we've added translation validation, so if the the feature flag was removed, it would still be production ready, at least up to our knowledge. |
atlas
step to the buildatlas
step to the build FC-0012
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 for bearing with me Omar!
Hey!! That's a great way to end the day. Thanks @regisb!
On the contrary! It's been a great review. I'm new to Tutor anyway, so I'm expecting a lot of comments anyway. |
@@ -31,6 +31,7 @@ | |||
"OAUTH2_KEY_SSO": "discovery-sso", | |||
"OAUTH2_KEY_SSO_DEV": "discovery-sso-dev", | |||
"CACHE_REDIS_DB": "{{ OPENEDX_CACHE_REDIS_DB }}", | |||
"ATLAS_PULL": False, |
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.
@OmarIthawi , shouldn't this be DISCOVERY_ATLAS_PULL
?
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.
@shadinaif the prefix is automatically added via Tutor. So in the config.yaml
file it will be DISCOVERY_ATLAS_PULL: true
.
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.
Thank you!
Add https://github.com/openedx/openedx-atlas to the Dockerfile and run the
atlas pull
command on every build.I'm starting here because this is a simple plugin and helps us to design it properly. The end goal is to add
atlas
on all other components including edX Platform, MFEs and possibly even ecommerce.This is a very minimal build that needs the following to be viable:
This PR requires the following branch of discovery: openedx/course-discovery#4037
TODO
Implement feat: Configure atlas using environment variables openedx/openedx-atlas#17Implement feat: Add default languages list inatlas
(in discussion) openedx/openedx-atlas#30DISCOVERY_ATLAS_OPTOINS
variable to keep it minimalBuilding and testing logs
Check the file tree
Verify translations within the image:
Background
This contribution is part of the FC-0012 project which is sparked by the Translation Infrastructure update OEP-58.