-
Notifications
You must be signed in to change notification settings - Fork 15
use is_ci_test in place of bench_name_ci #301
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
base: main
Are you sure you want to change the base?
Conversation
| def set_tag_ci(self): | ||
| """Set tag CI on smallest benchmark, so it can be selected on the cmd line via --tag CI""" | ||
| if self.benchmark_info[0] == 'hackathonGPU/benchmark': | ||
| self.tags.add(TAGS.CI) |
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 was completely puzzled: MetalWalls wasn't showing up if I did -t CI after your change. The issue is that MetalWalls does not inherit from EESSI_mixin. We should probably do that - but probably in a separate PR.
I'd just revert the change here, as self.is_ci_test is only respected by tests that inherit from EESSI_mixin.
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.
ah right, will revert
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.
done in ae1b734
|
Discussed in sync meeting: @casparvl will re-review by rerunning the tests. If those pass, it's good to go. |
is_ci_testis simpler than having to set bothbench_nameandbench_name_ci.it's also easier to understand and to explain :)
see also the companion docs PR:
is_ci_testinstead ofbench_name_cidocs#623