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

Starting tagging functionality #65

Merged
merged 62 commits into from
Mar 13, 2024
Merged

Starting tagging functionality #65

merged 62 commits into from
Mar 13, 2024

Conversation

pearce8
Copy link
Collaborator

@pearce8 pearce8 commented Dec 1, 2023

  • Set of tags for benchpark: A central place to keep all the possible tag values.
  • Need to be able to read tags in application.py, whether in benchpark/repo, or in the Ramble built-ins.
  • Need functionality to report on the overall tags covered by experiments.
  • Need functionality to report on the overall tags for each suite.
  • When adding new tags to experiments, should check on PR that a tag exists, and encourage the users to pick from existing tags. If a new tag is needed, propose to add it to the benchpark_tags.yaml before starting to use it in experiments.

@scheibelp scheibelp self-assigned this Dec 1, 2023
@scheibelp
Copy link
Collaborator

I think this should be automatically constructed from ramble attributes --tags --all. Note that this satisfies:

Need to be able to read tags in application.py, whether in benchpark/repo, or in the Ramble built-ins.

as long as the ramble config includes both of these repos.

Need functionality to report on the overall tags for each suite.

what is a suite? Is it just another tag that happens to be used for organization? For example I don't see a designated suite attribute for Ramble application objects.

If a new tag is needed, propose to add it to the benchpark_tags.yaml before starting to use it in experiments.

FWIW, one of our CI checks could be to auto-collect the tags and compare them with benchpark_tags.yaml, and fail automatically if they don't match. Then any PR would have to update benchpark_tags.yaml as part of the diff (we can likewise provide a command to do that update automatically.

bin/benchpark_tags.yaml Outdated Show resolved Hide resolved
@becker33
Copy link
Collaborator

becker33 commented Dec 5, 2023

Some tags seem to be per-application and some tags seem to be per-experiment (since applications can stress the system in different ways with different arguments. Should we check with the Ramble developers about adding a tagging capability to the ramble.yaml syntax.

These tags should then filter into benchpark list so that users can select tags for attributes of the system they want to test and get a list of potentially relevant experiments to choose from. Maybe benchpark list -t memory-bandwidth

Maybe we should publish (potentially outside of benchpark?) a taxonomy of benchmarking types which could be used as the set of available tags in benchpark? I'm not sure whether this should be coupled into benchpark or treated as an external dependency? I guess it's easier to start internal and spin out if necessary.

@becker33
Copy link
Collaborator

becker33 commented Dec 5, 2023

@scheibelp I think that we may want our list to be separate from the list of tags used in Ramble, because I think there are tags relevant to experiments that are not applicable at the application level.

I agree that we should have a CI check that all tags in our repo are in the list of tags, but I don't know that we want to necessarily care about every tag that ramble has in upstream packages. If someone tags an application as "google" because they developed it, we probably don't care from a benchpark perspective

@scheibelp
Copy link
Collaborator

I think that we may want our list to be separate from the list of tags used in Ramble, because I think there are tags relevant to experiments that are not applicable at the application level.

Agree, I think that was a possible reason for expanding the YAML format from a list of tags to a dict, which was collapsed in #65 (comment) (I should have made that a separate comment vs. an annotation).

I agree that we should have a CI check that all tags in our repo are in the list of tags, but I don't know that we want to necessarily care about every tag that ramble has in upstream packages. If someone tags an application as "google" because they developed it, we probably don't care from a benchpark perspective

Does this just mean that we might not want to display all tags that exist in all application.py's (and you think it makes sense to report a problem if a user mentions a tag that is not in an application.py, or possibly in a "group" as proposed above in this comment)? Without that grouping, I think tags outside of the set exported by applications aren't useful, unless you had some notion of another reason we could use them.

@pearce8 pearce8 added the feature New feature or request label Dec 19, 2023
@github-actions github-actions bot added the application New or modified application label Jan 9, 2024
@pearce8 pearce8 added this to the v0.1.1 milestone Jan 20, 2024
@pearce8 pearce8 linked an issue Jan 20, 2024 that may be closed by this pull request
@slabasan slabasan marked this pull request as ready for review February 22, 2024 06:23
@github-actions github-actions bot added the ci Involving Project CI & Unit Tests label Mar 13, 2024
@alecbcs alecbcs force-pushed the feature/tagging branch 2 times, most recently from e3c84c9 to 61ebff7 Compare March 13, 2024 21:52
@github-actions github-actions bot added the dependencies Modifications to a Dependency File label Mar 13, 2024
Copy link
Member

@alecbcs alecbcs left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@alecbcs alecbcs merged commit f3cfc47 into develop Mar 13, 2024
7 checks passed
@slabasan slabasan deleted the feature/tagging branch March 14, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application New or modified application ci Involving Project CI & Unit Tests dependencies Modifications to a Dependency File docs Improvements or additions to documentation experiment New or modified experiment feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

benchpark tags
5 participants