[AIR] Fix ResourceChangingScheduler not working with AIR
#26307
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Yard1
commented
Jul 6, 2022
|
@sumanthratna FYI this PR will conflict with yours |
amogkam
reviewed
Jul 11, 2022
| # Same check as in TrialRunner | ||
| if not (isinstance(self.fail_fast, bool) or self.fail_fast.upper() != "RAISE"): | ||
| raise ValueError( | ||
| "fail_fast must be one of {bool, 'raise'}. " f"Got {self.fail_fast}." |
There was a problem hiding this comment.
Suggested change
| "fail_fast must be one of {bool, 'raise'}. " f"Got {self.fail_fast}." | |
| "fail_fast" must be one of {bool, 'RAISE'}. " f"Got {self.fail_fast}." |
There was a problem hiding this comment.
We use lowercase in the docstring, so I figured we may as well be consistent here
| @@ -41,7 +42,7 @@ def trial_id(self) -> str: | |||
| return self._session.trial_info.id | |||
|
|
|||
| @property | |||
| def trial_resources(self) -> Dict[str, float]: | |||
| def trial_resources(self) -> "PlacementGroupFactory": | |||
There was a problem hiding this comment.
For my own understanding, is this a behavior change, or is this correcting the type hint?
There was a problem hiding this comment.
Behavior change. we agreed upon this with @xwjiang2010 - basically, just returning the dictionary of resources is not sufficient because we lose the bundle information. In the future we can make it return scaling configs instead, once we transition PGF into a fully developer API.
| @@ -178,7 +179,7 @@ def _validate_attributes(self): | |||
| if not isinstance(self.scaling_config, dict): | |||
| raise ValueError( | |||
| f"`scaling_config` should be an instance of `dict`, " | |||
| f"found {type(self.run_config)} with value `{self.run_config}`." | |||
| f"found {type(self.scaling_config)} with value `{self.scaling_config}`." | |||
|
Will make issues once this is merged! |
krfricke
approved these changes
Jul 12, 2022
10 tasks
Stefan-1313
pushed a commit
to Stefan-1313/ray_mod
that referenced
this pull request
Aug 18, 2022
…ct#26307) This PR ensures that the new trial resources set by `ResourceChangingScheduler` are respected by the train loop logic by modifying the scaling config to match. Previously, even though trials had their resources updated, the scaling config was not modified which lead to eg. new workers not being spawned in the `DataParallelTrainer` even though resources were available. In order to accomplish this, `ScalingConfigDataClass` is updated to allow equality comparisons with other `ScalingConfigDataClass`es (using the underlying PGF) and to create a `ScalingConfigDataClass` from a PGF. Please note that this is an internal only change intended to actually make `ResourceChangingScheduler` work. In the future, `ResourceChangingScheduler` should be updated to operate on `ScalingConfigDataClass`es instead of PGFs as it is now. That will require a deprecation cycle. Signed-off-by: Stefan van der Kleij <[email protected]>
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
This PR ensures that the new trial resources set by
ResourceChangingSchedulerare respected by the train loop logic by modifying the scaling config to match. Previously, even though trials had their resources updated, the scaling config was not modified which lead to eg. new workers not being spawned in theDataParallelTrainereven though resources were available.In order to accomplish this,
ScalingConfigDataClassis updated to allow equality comparisons with otherScalingConfigDataClasses (using the underlying PGF) and to create aScalingConfigDataClassfrom a PGF.Please note that this is an internal only change intended to actually make
ResourceChangingSchedulerwork. In the future,ResourceChangingSchedulershould be updated to operate onScalingConfigDataClasses instead of PGFs as it is now. That will require a deprecation cycle.Related issue number
Closes #26130
Checks
scripts/format.shto lint the changes in this PR.