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

ci: Merge similar try jobs when possible #31347

Merged
merged 5 commits into from Feb 16, 2024

Conversation

mrobinson
Copy link
Member

This comes up a lot when triggering wpt-2013 and wpt-2020. Instead of
merging the jobs and triggering both layouts, the try parser will
trigger two separate jobs. Running two of the same builds at once seems
to cause the CI to fail one of them [1]. This fixes that issue.

  1. An example of this: https://github.com/servo/servo/actions/runs/7892269495

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes

This comes up a lot when triggering wpt-2013 and wpt-2020. Instead of
merging the jobs and triggering both layouts, the try parser will
trigger two separate jobs. Running two of the same builds at once seems
to cause the CI to fail one of them [1]. This fixes that issue.

1. An example of this: https://github.com/servo/servo/actions/runs/7892269495
@sagudev
Copy link
Member

sagudev commented Feb 14, 2024

When triggering WPT for both layouts there is wpt preset, so just creating T-wpt label should run wpt on both layouts. That being said is still think it's good we do merging if possible.

The error you se are due to how artifacts are shared, that I hope will be solved with artifacts-v4 that I am working on.

Comment on lines 63 to 65
if self.workflow != other.workflow or self.profile != other.profile or \
self.wpt_tests_to_run != other.wpt_tests_to_run:
return False
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be error-prone when adding new fields in JobConfig. Maybe we could compare them as dictionaries and ignore some fields: https://stackoverflow.com/questions/10480806/compare-dictionaries-ignoring-specific-keys

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an interesting idea, but it seems a bit heavy-weight and people adding new fields to JobConfig will still have to remember to update this function. Instead I've created a new class variable which holds the fields that cannot differ for a merge to happen. This is near the fields and I've added doc to remind people to update the list.

In addition, I've added more tests, which should also help prevent mistakes in the future.

python/servo/try_parser.py Outdated Show resolved Hide resolved
python/servo/try_parser.py Outdated Show resolved Hide resolved
python/servo/try_parser.py Outdated Show resolved Hide resolved
python/servo/try_parser.py Outdated Show resolved Hide resolved
mrobinson and others added 2 commits February 14, 2024 18:05
Co-authored-by: Samson <16504129+sagudev@users.noreply.github.com>
python/servo/try_parser.py Outdated Show resolved Hide resolved
Copy link
Member

@sagudev sagudev left a comment

Choose a reason for hiding this comment

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

One suggestion remaining but otherwise looks good.

Co-authored-by: Samson <16504129+sagudev@users.noreply.github.com>
Copy link
Member Author

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Thanks for the review, @sagudev.

@mrobinson mrobinson added this pull request to the merge queue Feb 16, 2024
Merged via the queue into servo:main with commit 7e9be5a Feb 16, 2024
9 checks passed
@mrobinson mrobinson deleted the improve-try-parser branch February 16, 2024 13:56
@sagudev sagudev mentioned this pull request Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants