Page MenuHomePhabricator

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

Authored by schrockn on Oct 26 2020, 9:34 PM.

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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

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

383–391

confused by this - pending clean up?

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
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.

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

++

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

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

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