-
Notifications
You must be signed in to change notification settings - Fork 24
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
Feature/new modifier implementation #356
base: develop
Are you sure you want to change the base?
Conversation
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 have a suggestion on refactoring caliper.Caliper
into two classes to make it easier to assemble modifier/spec components.
) | ||
elif self.spec.satisfies("caliper=cuda"): | ||
cuda_support = ( | ||
self.spec.satisfies("caliper=cuda") and True |
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.
and True
looks vestigial
# set package spack specs | ||
package_specs = {} | ||
|
||
if not self.spec.satisfies("caliper=none"): |
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 get the impression that some combinations of caliper=...
are not allowed. I don't know if we support conflict
yet, but FWIW you can raise an exception here for forbidden combinations.
from benchpark.experiment import Experiment | ||
|
||
|
||
class Caliper(Experiment): |
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 think the failure at https://github.com/LLNL/benchpark/actions/runs/11446987606/job/31847369138?pr=356 will be resolved if you make it so that caliper
does not inherit Experiment
(which I think is what you wanted to do). If that doesn't work I'll likely have to investigate it (but that failure in the dry runs does in fact look legitimate, so we'll want to address it before merging this).
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 coulda also be addressed by undoing 2a0a077 I think (the part that rearranges the order of declared superclasses)
New modifier implementation using experiment.py