-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Bug 1622800 - part 11 to 14: Enable all locales and create hook #6459
Bug 1622800 - part 11 to 14: Enable all locales and create hook #6459
Conversation
ff1a075
to
d3b1caf
Compare
d3b1caf
to
d7de0fa
Compare
58dd85b
to
533b564
Compare
job: | ||
type: decision-task | ||
target-tasks-method: l10n_screenshots | ||
when: [] # Manual trigger only |
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 will give the link later after this lands.
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.
Here's the link https://firefox-ci-tc.services.mozilla.com/hooks/project-releng/cron-task-mozilla-mobile-firefox-ios%2Fl10-screenshots.
It doesn't work yet (see #6529). I'll send this link out once I know it's working.
@@ -256,7 +256,7 @@ tasks: | |||
~/.local/bin/taskgraph action-callback | |||
else: > | |||
PIP_IGNORE_INSTALLED=0 pip install --user /builds/worker/checkouts/taskgraph && | |||
PIP_IGNORE_INSTALLED=0 pip install --user arrow taskcluster pyyaml && |
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.
These dependencies aren't used.
@@ -21,7 +31,7 @@ fi | |||
|
|||
LOCALES=$* | |||
if [ $# -eq 0 ]; then | |||
LOCALES="af an anp ar ast az bg bn bo br bs ca co cs cy da de dsb el en-CA en-GB en-US eo es-AR es-CL es-MX es eu fa fi fr ga-IE gd gl gu-IN he hi-IN hr hsb hu hy-AM ia id is it ja jv ka kab kk km kn ko lo lt lv ml mr ms my nb-NO ne-NP nl nn-NO oc or pa-IN pl pt-BR pt-PT rm ro ru ses si sk sl sq su sv-SE ta te th tl tr uk ur uz vi zgh zh-CN zh-TW" |
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 list has been transferred to l10n-screenshots-locales.txt
so it can be used by taskgraph.
@@ -13,9 +13,10 @@ transforms: | |||
|
|||
jobs: | |||
screenshots: | |||
attributes: | |||
chunk_locales: ["en-US"] |
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 value is used by the bitrise transform down below.
provisioner: 'mobile-{level}' | ||
implementation: docker-worker | ||
os: linux | ||
worker-type: 'b-linux' | ||
worker-type: 'bitrise' |
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 created this new worker-type. It's a type defined for taskcluster. It currently maps our capacity in bitrise https://hg.mozilla.org/ci/ci-configuration/rev/39f5c268f49092e33278ee1ddd2bdf0cfb09db73#l1.33. If I kept using b-linux
, we would end up with 10 taskcluster workers all delegating work to bitrise as the same time. This means 7 of these workers would be polling Bitrise while knowing Bitrise not doing anything for them.
not-for-locales: | ||
- en-US # Already built as part of the build task | ||
|
||
locales-per-chunk: 6 |
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.
Here's how I came up with this number.
Our bitrise plan have a 90-minute-limit. Thanks to @isabelrios' great work in #6420, 6 locales takes between 50 to 60 minutes to complete when everything is fine. If some tests fail, xcode will rerun the failed tests, adding more minutes to the counter.
A 7th locale put some of jobs to 70 minutes, which only gives 20 minutes in case of a problem. So I think 6 locales is a sweet spot.
We have 92 locales which yields 16 chunks. Our bitrise plan has 3 concurrent builds. So, the full suite will take about 6 hours to complete. I think it's a great improvement based on the current situation 😃
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, I think so too...the time spent is reduced by more than half of the current time needed...
from ..screenshots_locales import get_screenshots_locales | ||
|
||
|
||
def loader(kind, path, config, params, loaded_tasks): |
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 function loads all locales and chunks them into jobs of 6 locales.
|
||
worker.setdefault("docker-image", {"in-tree": "screenshots"}) | ||
worker.setdefault("max-run-time", 3600) | ||
worker.setdefault("max-run-time", 10800) |
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 set the max-run-time to 3 hours. Basically, we may end up in situation where a TC worker schedules a job to bitrise, but the latter is overloaded (say because of the smoketests jobs which aren't monitored by Taskcluster). So the TC worker will schedule a job then wait for another to finish. Since a job is max 90 minutes, I set the limit to twice that value.
@@ -50,7 +50,7 @@ def sync_main( | |||
parser.add_argument("--branch", required=True, help="the git branch to generate screenshots from") | |||
parser.add_argument("--commit", required=True, help="the git commit hash to generate screenshots from") | |||
parser.add_argument("--workflow", required=True, help="the bitrise workflow to schedule") | |||
parser.add_argument("--locale", required=True, help="locale to generate the screenshots for") | |||
parser.add_argument("--locale", dest="locales", metavar="LOCALE", action="append", required=True, help="locale to generate the screenshots for (can be repeated)") |
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 change enables the script to be called this way: bitrise-schedule.py --locale en-CA --locale es --locale fr
@@ -173,9 +175,11 @@ async def download_log(client, build_slug): | |||
|
|||
response = await do_http_request_json(client, url) | |||
download_url = response["expiring_raw_log_url"] | |||
if not download_url: | |||
raise TaskException("Bitrise has no log to offer for job {}. Please check https://app.bitrise.io/app/{}".format(build_slug, BITRISE_APP_SLUG_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.
Because of this line, I ended up with a red job, even though Bitrise generated all locales. Their API just failed to provide us a link to their logs.
I also fixed the URL to point to the build, directly.
This was tested (with a limited number of locales) at https://firefox-ci-tc.services.mozilla.com/tasks/TBzi794DR0yeZ_OVS7HyAQ/runs/2. The next step to me, is to see if how it works in production 😃 Please let me know if you have any questions, I'm happy to explain anything 🙂 |
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.
LGTM! amazing work!! :)
@@ -21,7 +31,7 @@ fi | |||
|
|||
LOCALES=$* | |||
if [ $# -eq 0 ]; then | |||
LOCALES="af an anp ar ast az bg bn bo br bs ca co cs cy da de dsb el en-CA en-GB en-US eo es-AR es-CL es-MX es eu fa fi fr ga-IE gd gl gu-IN he hi-IN hr hsb hu hy-AM ia id is it ja jv ka kab kk km kn ko lo lt lv ml mr ms my nb-NO ne-NP nl nn-NO oc or pa-IN pl pt-BR pt-PT rm ro ru ses si sk sl sq su sv-SE ta te th tl tr uk ur uz vi zgh zh-CN zh-TW" | |||
LOCALES=$(cat "$PROJECT_DIR/l10n-screenshots-locales.txt") |
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.
Nice!
not-for-locales: | ||
- en-US # Already built as part of the build task | ||
|
||
locales-per-chunk: 6 |
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, I think so too...the time spent is reduced by more than half of the current time needed...
Thank you for submitting a pull request, your contributions are greatly appreciated!
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Please make sure you've run the test scheme and all the tests pass. If your changes affect existing tests please make sure to update the tests as well.