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

Added omnistat support for individual job steps and HPO trials. #276

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ashwinma
Copy link
Collaborator

This is a draft PR just to understand what code changes are needed to be made to enable omnistat on Frontier

@allaffa
Copy link
Collaborator

allaffa commented Sep 5, 2024

@ashwinma
This PR cannot be merged until the CI Test pass.
@jychoi-hpc
Is it better in terms of practice if we fix the formatting ourselves or if the author (Ashwin) fixes it himself?

@ashwinma
Copy link
Collaborator Author

ashwinma commented Sep 5, 2024

@allaffa I was not sure if you wanted to have this merged to main or just use this PR as a reference for you. That is why I marked this PR as "WIP"

Having said that, if there is interest in merging with main, we could probably check the system (if (system == frontier)) and then call omnistat.

I can do the code-formatting on my end with black if that is what you were hinting at.

Thoughts?

@allaffa
Copy link
Collaborator

allaffa commented Sep 5, 2024

@allaffa I was not sure if you wanted to have this merged to main or just use this PR as a reference for you. That is why I marked this PR as "WIP"

Having said that, if there is interest in merging with main, we could probably check the system (if (system == frontier)) and then call omnistat.

I can do the code-formatting on my end with black if that is what you were hinting at.

Thoughts?

@ashwinma I like the idea of adding a (if (system == frontier))
Instead of specifying Frontier, may it be possible to make the check more general to have it work on any system with AMD GPUs?

@jychoi-hpc what do you think?

@allaffa
Copy link
Collaborator

allaffa commented Sep 22, 2024

@allaffa I was not sure if you wanted to have this merged to main or just use this PR as a reference for you. That is why I marked this PR as "WIP"
Having said that, if there is interest in merging with main, we could probably check the system (if (system == frontier)) and then call omnistat.
I can do the code-formatting on my end with black if that is what you were hinting at.
Thoughts?

@ashwinma I like the idea of adding a (if (system == frontier)) Instead of specifying Frontier, may it be possible to make the check more general to have it work on any system with AMD GPUs?

@jychoi-hpc what do you think?

@jychoi-hpc
I never heard back frown you on this.

Please see conversation in this thread with @ashwinma and let us decide how to proceed.

@allaffa
Copy link
Collaborator

allaffa commented Oct 8, 2024

@jychoi-hpc
Does this PR need to be updated?

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