Skip to content
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 atlas step to the build - FC-0012 #22

Closed

Conversation

shadinaif
Copy link

Adding the pull behind a feature flag until it's fully implemented

Background

This contribution is part of the FC-0012 project which is sparked by the Translation Infrastructure update OEP-58.

Adding the pull behind a feature flag until it's fully implemented

Refs: FC-0012 OEP-58
@shadinaif shadinaif marked this pull request as ready for review November 26, 2023 08:54
@shadinaif
Copy link
Author

please review @regisb @OmarIthawi

@Faraz32123 Faraz32123 self-assigned this Nov 27, 2023
@Faraz32123 Faraz32123 self-requested a review November 27, 2023 08:52
Copy link
Collaborator

@regisb regisb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me, but I'm wondering what's the strategy here. For how many plugins are you planning on deploying atlas behind a feature flag? If Atlas works, then we should just add it to all plugins, with no feature flag. (and make it possible to customize translations) If it doesn't, then surely we don't need more than one or two plugins to realize that, right?

@OmarIthawi
Copy link
Contributor

OmarIthawi commented Nov 28, 2023

This change looks good to me, but I'm wondering what's the strategy here. For how many plugins are you planning on deploying atlas behind a feature flag? If Altas works, then we should just add it to all plugins, with no feature flag. (and make it possible to customize translations) If it doesn't, then surely we don't need more than one or two plugins to realize that, right?

Good point.

Atlas currently work and only valid translations are making it to main branch, which now makes not lesser than the pre-OEP-58 workflows. It has about 25 languages already, so we don't know any features remaining from merging into Quince+.

That's a good change since the last time we worked on the tutor-discovery.

What we lack now is testing from anyone who's on a recent release which we lack access to. The most recent production/staging release we are working on is on Maple.

Nevertheless, I think we can remove the feature flag for credentials and experiment with atlas pull enabled by default. What do you think?

@regisb
Copy link
Collaborator

regisb commented Nov 28, 2023

I'm glad to hear that Atlas now has translation parity with pre-OEP-58! Before we can enable Atlas by default, I'd like to know the following:

  • Does it work for all major Open edX applications? edx-platform, forum, MFEs, notes, ecommerce, discovery.
  • How can users extend and customize translation strings to replace this procedure for edx-platform and that one for MFEs?
  • What's the list of languages supported? (we should replace that list in tutor core)

We should try to enable Atlas in time for Quince (Dec. 10th). But if we don't, that's fine too, we can always merge it later.

Nevertheless, I think we can remove the feature flag for credentials and experiment with atlas pull enabled by default. What do you think?

Yes, we can, but in nightly or quince, not in master. (this PR currently targets the "main" branch)

@OmarIthawi
Copy link
Contributor

  • Does it work for all major Open edX applications? edx-platform, forum, MFEs, notes, ecommerce, discovery.

Currently only MFE Communication and discovery are fully functional. Others are work in progress.

This use case needs more study. Our goal is at least not break what Tutor have at the moment if the old Transifex project was used.

We argue that this use case will be needed less due to the low turnaround time between Transifex and atlas pull as opposed to getting translations PRs merged into an old open-release branch.

Nevertheless, the edx-platform customization should work regardless of how the translations is being pulled because it uses LOCALE_PATHS.

MFEs customization isn't compatible at the moment with atlas pull though which is a known issue.

  • What's the list of languages supported? (we should replace that list in tutor core)

This is the full list of supported languages:

More support will be added in-line with the Transifex Working Groups.

Yes, we can, but in nightly or quince, not in master. (this PR currently targets the "main" branch)

@regisb @shadinaif yes let's use the nightly branch for this pull request.

As for a timeline, we'd like to perform the cutover in January.

@regisb
Copy link
Collaborator

regisb commented Dec 11, 2023

It is absolutely essential that users are able to customise the translation strings. We need the following features:

  1. Change, add and remove translation strings for supported Atlas languages. Trust me: people will want to modify these strings, even if they are updated quickly in Atlas.
  2. Same for MFEs.
  3. Add new languages, from the list of [already supported languages](https://github.com/overhangio/tutor
    /blob/65ba0d2de24a1ac0e9e91404dcd28eed9cbfd9a1/tutor/interactive.py#L58).

Until we have these features, I propose that we focus on one or two IDAs. I don't see the point of deploying Atlas in all Tutor plugins. Let's integrate Atlas in the nightly branch of one or two plugins. Once the above items are resolved (and we can help with that), we can generalize to other plugins.

Does that make sense?

@OmarIthawi
Copy link
Contributor

It is absolutely essential that users are able to customise the translation strings. We need the following features:

  1. Change, add and remove translation strings for supported Atlas languages. Trust me: people will want to modify these strings, even if they are updated quickly in Atlas.

Makes sense. edx-platform support for customized strings to be retained. No issues with that.

  1. Same for MFEs.

MFEs for overriding languages support can be added, but it needs a bit more work.

  1. Add new languages, from the list of already supported languages.

atlas pull won't interfere with the Tutor list of languages. atlas pull will pull anything that's available in https://github.com/openedx/openedx-translations repo. Whether a particular language is supported or not, it's parallel non-blocking discussion to this work happening in #wg-translations.

Until we have these features, I propose that we focus on one or two IDAs. I don't see the point of deploying Atlas in all Tutor plugins. Let's integrate Atlas in the nightly branch of one or two plugins. Once the above items are resolved (and we can help with that), we can generalize to other plugins.

Does that make sense?

Some of the points makes sense, but the timing doesn't. The old Transifex project is planned to be set as read-only after Quince is cut. Same for in-repo strings for all MFEs and Python repositories. So we need to ship those features with or without a flag to prepare for the cutover, otherwise the Tutor nightly and anything after Quince translations will be outdated.

To summarize, I still recommend shipping those features behind a flag in nightly as soon as possible to keep Tutor up to date with the Transifex projects changes happening in January 2024 after Quince. Whether we want to ship them without a feature flag and enabled by default is an option we're open to go for if the Tutor teams support that.

cc: @brian-smith-tcril for timeline and old languages discussions.

@regisb regisb assigned regisb and unassigned Faraz32123 Dec 12, 2023
@regisb
Copy link
Collaborator

regisb commented Dec 14, 2023

So we need to ship those features with or without a flag to prepare for the cutover, otherwise the Tutor nightly and anything after Quince translations will be outdated.

I understand the urgency. What you are saying is that it's preferable to break existing customization features (in nightly) rather than to have outdated translation strings. I disagree with this approach for the following reason. We are going to open 13 different pull requests (one for each official plugin) to run Atlas there. Then we are going to think about a way for Tutor users to customize their translation strings. After we figure out how to do that, we are going to open 13 new pull requests to implement this mechanism. That's a total of 26 PRs. What I propose instead is that we design a comprehensive extension mechanism right now and try it out on a single repo. Once we are confident we have the right approach, we generalize Atlas to all plugins. And since we know for a fact that Atlas will be in Redwood, there's no need to hide Atlas behind a feature flag at all.

How does that sound? If you disagree let's schedule a meeting to talk about this.

@OmarIthawi
Copy link
Contributor

OmarIthawi commented Dec 15, 2023

What you are saying is that it's preferable to break existing customization features (in nightly) rather than to have outdated translation strings.

Not necessarily. We can ship most of the plugins work in a backward compatible way. The only thing that's going to break, if we enable another MFE without re-ordering the atlas pull step to run before i18n merge. Re-ordering the steps would address your concern to ensure i18n-merge.js works with more MFEs.

For all other repositories the overriding the translations feature would be maintained including this Credential atlas pull pull request. This is possible due to Python LOCALE_PATHS which MFEs don't support.

My question is, are we open to changing the i18n-merge feature and cleaning up the https://github.com/openedx/openedx-i18n work if we can address the same Developer need, i.e. overriding translations?

Back to the feature flags. The reason we're keeping the feature flags is because we'd like to ship the feature disabled by default until the Transifex project cutover, happening in two weeks.

What I'm seeing is two options in both this pull request and other open ones:

  • Option 1: Merge the pull requests now, with feature flags disabled. Then after the cutover we remove the feature flags in new pull requests.
  • Option 2: Keep the pull requests open. Remove the feature flags. Perform the Transifex cutover and merge the pull requests.

And since we know for a fact that Atlas will be in Redwood, there's no need to hide Atlas behind a feature flag at all.

It appears to me that you're preferring Option 2. I think that's reasonable.

What I propose instead is that we design a comprehensive extension mechanism right now and try it out on a single repo. Once we are confident we have the right approach, we generalize Atlas to all plugins. And since we know for a fact that Atlas will be in Redwood, there's no need to hide Atlas behind a feature flag at all.

atlas pull is mostly one liner in multiple repositories, excluding the feature flag. I recall discussing the possibility of creating a plugin and dismissing that option because translations is a core part of the platform so it makes less sense to use a plugin for that.

How does that sound? If you disagree let's schedule a meeting to talk about this.

Let's schedule a meeting if we non of the options I've suggested makes sense.

@OmarIthawi
Copy link
Contributor

To conclude our discussion, I'm opening other two PRs:

@OmarIthawi
Copy link
Contributor

@shadinaif thanks for your work here. As you can see Tutor/Atlas integration has gone pretty off the the original plan and this needs to be closed in favor of #29

@Faraz32123 please close this pull request.

Thanks everyone for the feedback.

@Faraz32123
Copy link
Collaborator

closing this PR in favor of #29.

@Faraz32123 Faraz32123 closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Won't fix
Development

Successfully merging this pull request may close these issues.

4 participants