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

Adds a multi_node_queue_name ci config option #504

Merged
merged 2 commits into from
Feb 18, 2025
Merged

Conversation

hategan
Copy link
Collaborator

@hategan hategan commented Feb 14, 2025

If specified, this option allows multi-node tests to run using a different queue than the one in queue_name. The wisdom of this is that on certain systems it makes sense to use more restrictive but less expensive queues by default, and only use more expensive, multi-node queues when strictly needed. In a sense, this could be hacked using custom_attributes and test filters, which some deployments do, but it is awkward and difficult to set up in a way that predicts all future multi-node tests.

When merged, the new ci_runner.py will automatically be used by all deployed tests. It was modified to not pass config
options that are not set, and all such options should have defaults in the conftest.py arg parser, both old and new.

This addition will affect tests that are currently deployed as follows (new and old refer to post and pre merging this PR):

  1. old_clone -> new_pr: OK (new PRs work with the old options)
  2. old_clone -> old_pr: OK (has old config and new ci_runner does not pass missing options)
  3. new_clone -> old_pr: Not OK (will have to update PR - only one ATM; will pass new option which is not recognized)
  4. new_clone -> new_pr: OK by construction

allows multi-node tests to run using a different queue than the one
in `queue_name`.
@hategan hategan requested a review from andre-merzky February 14, 2025 20:23
Copy link
Collaborator

@andre-merzky andre-merzky left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me - thanks!

@hategan
Copy link
Collaborator Author

hategan commented Feb 18, 2025

This looks reasonable to me - thanks!

Thank you!

@hategan hategan merged commit 12e349a into main Feb 18, 2025
14 checks passed
@hategan hategan deleted the ci_add_multi_node_queue branch February 18, 2025 21:56
hategan added a commit that referenced this pull request Feb 19, 2025
hategan added a commit that referenced this pull request Feb 19, 2025
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.

2 participants