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

[DS][x/n] Allow SchedulingConditions to target specific dependency keys #21854

Conversation

OwenKephart
Copy link
Contributor

@OwenKephart OwenKephart commented May 14, 2024

Summary & Motivation

This is a long-standing request. It's often useful to be able to detect the status of some specific set of dependency keys, rather than needing to treat all dependencies identically.

This creates a single structure for defining and resolving these sorts of collections, but does not export it as a top-level user-facing concept, as that feels a bit like jumping the gun.

This allows users to explicitly select any keys they want to select for, as well as explicitly ignore some specific keys, as either workflow is potentially useful. An explicit choice was made to not error when an "invalid" selection (i.e. one that references keys that are not dependencies) is made, as this would be pretty annoying (imo).

How I Tested These Changes

Copy link
Contributor Author

OwenKephart commented May 14, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @OwenKephart and the rest of your teammates on Graphite Graphite

@OwenKephart OwenKephart requested a review from schrockn May 14, 2024 22:30
Comment on lines 21 to 22
if not self.dep_selection.is_all:
description += f" within selection: {self.dep_selection.description}"
return description
Copy link
Member

Choose a reason for hiding this comment

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

nit: could be one-liner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe missing something dumb, but I'm not seeing a clean way to make this a oneliner, e.g.:

return "All deps" + (f"within _selection: {self.dep_selection}" if self.dep_selection else "")

would work, but it feels a fair bit harder to read to me. Open to suggestions though

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

We need to figure out a way to make this compose better IMO

@OwenKephart OwenKephart force-pushed the 05-14-Allow_conditions_which_target_dependencies_to_target_specific_dependencies branch from b75b319 to 0ec11e5 Compare May 16, 2024 22:45
@OwenKephart OwenKephart force-pushed the 05-14-Rename_AssetConditionScenarioState branch from f66fb78 to c100c16 Compare May 16, 2024 23:35
@OwenKephart OwenKephart force-pushed the 05-14-Allow_conditions_which_target_dependencies_to_target_specific_dependencies branch from 0ec11e5 to 6b48cc1 Compare May 16, 2024 23:35
@OwenKephart OwenKephart force-pushed the 05-14-Rename_AssetConditionScenarioState branch from c100c16 to ae60aef Compare May 17, 2024 21:35
@OwenKephart OwenKephart force-pushed the 05-14-Allow_conditions_which_target_dependencies_to_target_specific_dependencies branch from 6b48cc1 to d06ca84 Compare May 17, 2024 21:35
@OwenKephart OwenKephart force-pushed the 05-14-Allow_conditions_which_target_dependencies_to_target_specific_dependencies branch from d06ca84 to 1bc7987 Compare May 17, 2024 21:49
@OwenKephart OwenKephart requested a review from schrockn May 17, 2024 21:51
Copy link
Contributor Author

@schrockn Updated this to just work off of regular-old AssetSelections, and created a common interface thing so we don't have to implement get_dep_keys() multiple times.

@OwenKephart OwenKephart force-pushed the 05-14-Rename_AssetConditionScenarioState branch from c3080f8 to 8f3fbc9 Compare May 20, 2024 23:21
@OwenKephart OwenKephart force-pushed the 05-14-Allow_conditions_which_target_dependencies_to_target_specific_dependencies branch from 1bc7987 to abdf548 Compare May 20, 2024 23:21
@OwenKephart OwenKephart force-pushed the 05-14-Rename_AssetConditionScenarioState branch 3 times, most recently from 06e224c to 83934b5 Compare May 29, 2024 00:03
@OwenKephart OwenKephart force-pushed the 05-14-Rename_AssetConditionScenarioState branch from 83934b5 to 1b5c3d4 Compare May 29, 2024 22:29
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.

None yet

2 participants