Page MenuHomeElementl
Differential D4912

(make-pipeline-a-solid-4) Rename ISolidDefinition to NodeDefinition and spread name through composition process for code clarity
ClosedPublic
Actions

Authored by • schrockn on Oct 26 2020, 9:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 24, 1:38 PM
Unknown Object (File)
Thu, Nov 17, 7:01 PM
Unknown Object (File)
Thu, Nov 17, 1:57 PM
Unknown Object (File)
Sat, Nov 12, 4:56 PM
Unknown Object (File)
Sat, Nov 12, 4:16 PM
Unknown Object (File)
Nov 9 2022, 2:39 PM
Unknown Object (File)
Nov 6 2022, 8:05 AM
Unknown Object (File)
Nov 4 2022, 11:00 PM
View All 94 Files
Subscribers
None

Details

Reviewers
alangenfeld
• max
• sandyryza
Commits
R1:1aa474e04ccd: (make-pipeline-a-solid-4) Rename ISolidDefinition to NodeDefinition and spread…
Summary

With the ability to embed pipelines/graphs within composition,
the use of the term "solid" through the process becomes more
problematic.

This renames ISolidDefinition to NodeDefinition to indicate something
that can embedded in a graph (which can be a graph or a solid) and
begins to rename the internal implementations to be "nodes" instead of
"solids". Otherwise variables with the name "solid" could actually
refer to pipelines/graph.

Depends on D4888

Test Plan

BK

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• schrockn created this revision.Oct 26 2020, 9:34 PM
• schrockn updated this revision to Diff 24493.Oct 26 2020, 9:43 PM

up

Harbormaster failed remote builds in B20178: Diff 24492!Oct 26 2020, 9:51 PM
• schrockn updated this revision to Diff 24494.Oct 26 2020, 9:53 PM

up

• schrockn updated this revision to Diff 24495.Oct 26 2020, 9:56 PM

up

Harbormaster failed remote builds in B20179: Diff 24493!Oct 26 2020, 10:00 PM
• schrockn updated this revision to Diff 24496.Oct 26 2020, 10:03 PM

up

Harbormaster failed remote builds in B20180: Diff 24494!Oct 26 2020, 10:11 PM
Harbormaster failed remote builds in B20181: Diff 24495!
• schrockn updated this revision to Diff 24497.Oct 26 2020, 10:12 PM

up

• schrockn updated this revision to Diff 24498.Oct 26 2020, 10:20 PM

up

Harbormaster failed remote builds in B20182: Diff 24496!Oct 26 2020, 10:25 PM
Harbormaster failed remote builds in B20183: Diff 24497!Oct 26 2020, 10:29 PM
• schrockn updated this revision to Diff 24502.Oct 26 2020, 10:30 PM
• schrockn retitled this revision from (make-pipeline-a-solid-4) Rename ISolidDefinition to NodeDefinition and spread name appropriatley through composition process to (make-pipeline-a-solid-4) Rename ISolidDefinition to NodeDefinition and spread name through composition process for code clarity.
• schrockn edited the summary of this revision. (Show Details)
• schrockn added reviewers: alangenfeld, • max, • sandyryza.

up

• schrockn added inline comments.Oct 26 2020, 10:34 PM
python_modules/dagster/dagster/core/definitions/composition.py
256–257

@alangenfeld ran into this and it was confusing since variable was reassigned. not sure what the new name represents

Harbormaster completed remote builds in B20184: Diff 24498.Oct 26 2020, 10:38 PM
• schrockn published this revision for review.Oct 26 2020, 10:43 PM
Harbormaster completed remote builds in B20188: Diff 24502.Oct 26 2020, 11:00 PM
alangenfeld added a comment.Oct 27 2020, 3:39 PM

feeling good about this - going to take another pass

python_modules/dagster/dagster/core/definitions/composition.py
256–257

this is auto-aliasing at work, so maybe

original_node_name & resolved_node_name
starting_node_name & final_node_name

or something in that zone.

The re-assign is clowny so good catch and would be good to have two different names

384–392

confused by this - pending clean up?

alangenfeld mentioned this in D4847: (make-pipeline-a-solid-1) Allow pipeline to be used as composite_solid.Oct 27 2020, 3:55 PM
• schrockn updated this revision to Diff 24543.Oct 27 2020, 4:15 PM

up

Harbormaster completed remote builds in B20226: Diff 24543.Oct 27 2020, 4:38 PM
alangenfeld accepted this revision.Oct 27 2020, 4:41 PM

We should just be careful about leaving any thing solid that should be node as we work through this. Might be worth doing some book keeping in a place accessible by others incase you get pulled in to some other stuff before its seen through.

python_modules/dagster/dagster/core/definitions/composition.py
171–172

SolidInvocation is potentially reasonable to rename too right?

python_modules/dagster/dagster/core/definitions/pipeline.py
349–365

deal with deprecating / replacing these later i presume? I do feel like the external use of these is limited enough (or non existent) that a breaking change might be acceptable

This revision is now accepted and ready to land.Oct 27 2020, 4:41 PM
• schrockn added inline comments.Oct 27 2020, 4:59 PM
python_modules/dagster/dagster/core/definitions/pipeline.py
365

Yes I should have mentioned that. I wanted to do public API migrations in a sep diff and do a deprecation. I'm newly sensitive to the impact of breaking changes on users and I want to respect the policy we put forth at 0.9.0.

• max added inline comments.Oct 27 2020, 6:43 PM
python_modules/dagster/dagster/core/definitions/composition.py
171–172

++

• schrockn added inline comments.Oct 27 2020, 7:08 PM
python_modules/dagster/dagster/core/definitions/composition.py
172–173

yeah. let me do that on top of this one in stack as it is quite involved and this diff already large

• schrockn added inline comments.Oct 27 2020, 8:13 PM
python_modules/dagster/dagster/core/definitions/composition.py
172–173

btw SolidInvocation *is* a public api so this is more involved

• schrockn added a child revision: D4926: (make-pipeline-a-solid-5) NodeInvocation and SolidInvocation.Oct 27 2020, 8:48 PM
Closed by commit R1:1aa474e04ccd: (make-pipeline-a-solid-4) Rename ISolidDefinition to NodeDefinition and spread… (authored by • schrockn). · Explain WhyOct 27 2020, 8:58 PM
This revision was automatically updated to reflect the committed changes.
• schrockn added a commit: R1:1aa474e04ccd: (make-pipeline-a-solid-4) Rename ISolidDefinition to NodeDefinition and spread….

Revision Contents
Changeset List

PathSize
python_modules/
dagster/
dagster/
core/
definitions/
2 lines
139 lines
6 lines
8 lines
39 lines
38 lines
22 lines
51 lines
14 lines
4 lines
utils/
test/
4 lines
dagster_tests/
core_tests/
definitions_tests/
2 lines

Diff 24594

View Options

python_modules/dagster/dagster/core/definitions/__init__.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/composition.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/config_mappable.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/dependency.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/environment_configs.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/graph.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/i_solid_definition.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/pipeline.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/solid.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/solid_container.py

Loading...
View Options

python_modules/dagster/dagster/utils/test/__init__.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_composition.py

Loading...