[DS][30/n] Create new unique ids for each child based off of index #21671
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
This was referenced May 6, 2024
OwenKephart
changed the title
OwenKephart
force-pushed
the
05-06-Mark_SchedulingCondition_as_experimental
branch
from
9aebb7f
to
ced1666
Compare
OwenKephart
force-pushed
the
05-06-Static_constructor_for_eager_condition
branch
from
51c95bf
to
287734c
Compare
OwenKephart
force-pushed
the
05-06-Mark_SchedulingCondition_as_experimental
branch
from
ced1666
to
cb53c68
Compare
OwenKephart
force-pushed
the
05-06-Static_constructor_for_eager_condition
branch
from
287734c
to
fb609ea
Compare
This was referenced May 7, 2024
schrockn
reviewed
May 9, 2024
| @@ -73,7 +73,7 @@ def create( | |||
| return SchedulingContext.model_construct( | |||
| candidate_slice=asset_graph_view.get_asset_slice(asset_key), | |||
| condition=scheduling_condition, | |||
| condition_unique_id=scheduling_condition.get_unique_id(None), | |||
| condition_unique_id=scheduling_condition.get_unique_id(None, None), | |||
schrockn
reviewed
May 9, 2024
| @@ -25,9 +25,9 @@ def description(self) -> str: | |||
| def evaluate(self, context: SchedulingContext) -> SchedulingResult: | |||
There was a problem hiding this comment.
[Re: line 15]
Question: does the `Sequence` type in Python guarantee stable ordering? We might want to consider typing this as `List` which for sure guarantees that. We're relying on index for identity so just want to be sure.
See this comment inline on Graphite.
Question: does the `Sequence` type in Python guarantee stable ordering? We might want to consider typing this as `List` which for sure guarantees that. We're relying on index for identity so just want to be sure.
See this comment inline on Graphite.
There was a problem hiding this comment.
From what I can tell, the answer is yes -- I think in general if an object supports index operations (which all Sequences do), then ordering must be stable.
schrockn
requested changes
May 9, 2024
OwenKephart
force-pushed
the
05-03-Create_UpdatedSinceChildCondition
branch
from
b39eb21
to
aa9b7f1
Compare
OwenKephart
force-pushed
the
05-06-Static_constructor_for_eager_condition
branch
from
b835cb7
to
8158240
Compare
This was referenced May 10, 2024
OwenKephart
force-pushed
the
05-03-Create_UpdatedSinceChildCondition
branch
from
aa9b7f1
to
393055f
Compare
OwenKephart
force-pushed
the
05-06-Static_constructor_for_eager_condition
branch
from
8158240
to
3c56fea
Compare
OwenKephart
force-pushed
the
05-03-Create_UpdatedSinceChildCondition
branch
from
393055f
to
0da2923
Compare
OwenKephart
force-pushed
the
05-06-Static_constructor_for_eager_condition
branch
from
3c56fea
to
be7aa64
Compare
OwenKephart
force-pushed
the
05-03-Create_UpdatedSinceChildCondition
branch
from
0da2923
to
0d2ac17
Compare
OwenKephart
force-pushed
the
05-06-Static_constructor_for_eager_condition
branch
from
be7aa64
to
b721c23
Compare
schrockn
approved these changes
May 14, 2024
OwenKephart
force-pushed
the
05-03-Create_UpdatedSinceChildCondition
branch
from
0d2ac17
to
96789db
Compare
OwenKephart
force-pushed
the
05-06-Static_constructor_for_eager_condition
branch
from
b721c23
to
f64a604
Compare
OwenKephart
force-pushed
the
05-03-Create_UpdatedSinceChildCondition
branch
from
96789db
to
bc62b76
Compare
OwenKephart
force-pushed
the
05-06-Static_constructor_for_eager_condition
branch
from
f64a604
to
5033665
Compare
OwenKephart
force-pushed
the
05-03-Create_UpdatedSinceChildCondition
branch
from
bc62b76
to
e5a2f0d
Compare
OwenKephart
force-pushed
the
05-06-Static_constructor_for_eager_condition
branch
from
5033665
to
d09af0c
Compare
Merge activity
|
OwenKephart
force-pushed
the
05-03-Create_UpdatedSinceChildCondition
branch
2 times, most recently
from
7ec169c
to
2013a99
Compare
Base automatically changed from
05-03-Create_UpdatedSinceChildCondition
to
master
May 14, 2024 14:50
OwenKephart
force-pushed
the
05-06-Static_constructor_for_eager_condition
branch
from
d09af0c
to
80939db
Compare
danielgafni
pushed a commit
to danielgafni/dagster
that referenced
this pull request
Jun 18, 2024
…agster-io#21671) ## Summary & Motivation This fixes an issue I noticed when experimenting with the PR above this (creating the eager condition). If we base an evaluation node's id off of purely its description and its parent id, then we run into an issue if you have two children with the same description (but different sub-conditions). This new scheme ensures that all nodes in the graph will have unique ids, as no two nodes can have the same parent id, description, AND index within that parent ## How I Tested These Changes
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.
Summary & Motivation
This fixes an issue I noticed when experimenting with the PR above this (creating the eager condition).
If we base an evaluation node's id off of purely its description and its parent id, then we run into an issue if you have two children with the same description (but different sub-conditions).
This new scheme ensures that all nodes in the graph will have unique ids, as no two nodes can have the same parent id, description, AND index within that parent
How I Tested These Changes