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

remove roles to merge training role rules with training tag rules #1993

Conversation

cat-bro
Copy link
Collaborator

@cat-bro cat-bro commented Jun 6, 2024

For consideration: The aim is that somebody could add the tag 'training' to their history and get the same treatment as somebody in a training group, which could be useful for testing a tutorial ahead of time.

For issue #1942

@cat-bro cat-bro marked this pull request as ready for review August 1, 2024 02:28
@cat-bro cat-bro changed the title [WIP] remove roles to merge training role rules with training tag rules remove roles to merge training role rules with training tag rules Aug 1, 2024
@cat-bro cat-bro requested a review from nuwang August 1, 2024 02:29
nuwang
nuwang previously approved these changes Aug 2, 2024
Copy link
Contributor

@nuwang nuwang 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 good to me.

include_tags = ['pulsar']
exclude_tags = ['pulsar-training-large', 'training-exempt']
slurm_preferred_for_training = pulsar_score is not None and pulsar_score < pulsar_score_slurm_threshold
training_job_matches_specs = (not slurm_preferred_for_training) and helpers.tag_values_match(entity, include_tags, exclude_tags)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since only this line is different between slurm and pulsar, maybe it would make sense to move this code to a seperate file that we just import. A possible future enhancement. I imagine more scenarios like this will pile up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree - most of the code in the 3 blocks is identical.

@cat-bro cat-bro force-pushed the remove_roles_to_merge_training_role_rules_with_training_tag_rules branch from e2fb865 to eccf27b Compare August 5, 2024 03:07
@cat-bro cat-bro merged commit 5ac998b into usegalaxy-au:master Aug 5, 2024
1 check passed
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