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

Move environment configuration to its own class #784

Merged
merged 13 commits into from
Dec 15, 2023

Conversation

b-butler
Copy link
Member

@b-butler b-butler commented Nov 2, 2023

Description

Adds three classes

  • _PartitionConfig: an environment's partition configuration
  • _Partition: a specific partition's data
  • _NodeTypes: what kind of nodes a partition supports
    • SHARED: 1 or fewer nodes
    • MIXED: any number of nodes/processes
    • WHOLENODE: submit in whole nodes only

Motivation and Context

With recent changes to flow's template/submission logic (see #722), the environment classes were becoming loaded with class variables. This makes a logical change to storing the partition specific information in appropriate classes. This also removes some methods from the environments in favor of putting them in the partition classes.

Checklist:

@b-butler b-butler requested review from a team as code owners November 2, 2023 20:14
@b-butler b-butler requested review from cbkerr and shihkual and removed request for a team November 2, 2023 20:14
Also move threshold to _PartitionConfig class and gpus_per_node default
to 0.
@b-butler
Copy link
Member Author

b-butler commented Nov 2, 2023

Tested appropriate errors for GPU/non-GPU and node requests on Bridges2.

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f69aeb5) 69.41% compared to head (301ee3b) 69.54%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #784      +/-   ##
==========================================
+ Coverage   69.41%   69.54%   +0.13%     
==========================================
  Files          48       48              
  Lines        4407     4417      +10     
  Branches     1065     1069       +4     
==========================================
+ Hits         3059     3072      +13     
+ Misses       1140     1138       -2     
+ Partials      208      207       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joaander joaander mentioned this pull request Nov 3, 2023
@b-butler
Copy link
Member Author

@cbkerr when you have a moment could you review?

@b-butler
Copy link
Member Author

b-butler commented Dec 5, 2023

@cbkerr will you have time to review or should I assign someone else?

@cbkerr
Copy link
Member

cbkerr commented Dec 5, 2023

@b-butler Thanks for the ping. I can get to this during the next few days.

flow/environment.py Outdated Show resolved Hide resolved
flow/environments/umich.py Show resolved Hide resolved
flow/environment.py Outdated Show resolved Hide resolved
@b-butler b-butler requested a review from cbkerr December 11, 2023 20:09
Copy link
Member

@cbkerr cbkerr left a comment

Choose a reason for hiding this comment

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

Concerning documentation:

Are there any user-facing changes to the template and environment documentation we would need to add?

What are the risks of default values changing and messing up inherited values?

@b-butler
Copy link
Member Author

@cbkerr Nope, this is purely a developer facing thing currently.
And the way the model works is that the defaults are inherited as class members from _ComputeEnvironment but you can't partially inherit meaning you must explicitly set the full specification for every new class.

@b-butler b-butler merged commit 64e3ed1 into main Dec 15, 2023
11 checks passed
@b-butler b-butler deleted the refactor/environment-config branch December 15, 2023 19:47
@b-butler b-butler added this to the v0.27.0 milestone Jan 15, 2024
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