[RFC] allow different nodes in the same repo to have the same name

[sryza]

trafficstars

Summary & Motivation

Since time immemorial, Dagster has had the constraint that node (i.e. op and graph) names within a repository must be unique. I.e. you can't have two ops with different implementations that share a name.

In the past, when Dagit had more of an IDE vibe, we had good reason for this. Ops played a much more central role in the UI. There were views in the UI that allowed you to see the list of ops defined in a repository. Ops were expected to be widely reusable.

From what I can tell, Dagster no longer has these views, and there isn't anywhere in the UI that expects ops to have unique names.

At the same time, this constraint has been a stumbling block in a few different situations:

• https://github.com/dagster-io/dagster/issues/8076
• https://github.com/dagster-io/dagster/issues/17190
• https://github.com/dagster-io/dagster/issues/12406
• https://github.com/dagster-io/dagster/issues/9436#issuecomment-1445216199

This PR proposes removing the constraint.

How I Tested These Changes

dagster dev -f on the below code snippet, and then clicked around the UI, launched runs.

from dagster import op, job, Definitions


def make_op1():
    @op
    def op1():
        ...

    return op1


def make_op1_2():
    @op
    def op1():
        ...

    return op1


@job
def job1():
    make_op1()()


@job
def job2():
    make_op1_2()()


defs = Definitions(jobs=[job1, job2])

[alangenfeld]

edit: this is for within-a-job scoping which this PR doesn't change

JobSnapshot uses definitions name as a unique key to point at the backing definition for a node in the dependency structure https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/_core/snap/dep_snapshot.py#L212

I expect that if you added different description or some other property to the conflicting ops in your test plan that it would appear in the UI as if the same op were used in both jobs, with the "last" definition winning:

https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/_core/host_representation/job_index.py#L45

[sryza]

@alangenfeld this PR maintains the constraint that, within a job, nodes with the same name must have the same definition. Although I'm interested in relaxing that eventually as well.

I expect that if you added different description or some other property to the conflicting ops in your test plan that it would appear in the UI as if the same op were used in both jobs, with the "last" definition winning:

I just tested this out, and the correct descriptions are rendered.

[alangenfeld]

oops ya got my self confused on the scoping

[alangenfeld]

OK for the correct repository level scoping - usedSolids is the repository level graphql that exposes definitions and callsites
https://github.com/dagster-io/dagster/blob/master/python_modules/dagster-graphql/dagster_graphql/schema/external.py#L238-L239 and leans on the current assumptions here https://github.com/dagster-io/dagster/blob/master/python_modules/dagster-graphql/dagster_graphql/implementation/fetch_solids.py#L27-L37

I do see callsites in frontend code:

• https://github.com/dagster-io/dagster/blob/master/js_modules/dagster-ui/packages/ui-core/src/ops/OpsRoot.tsx - this appears to still be mounted via WorkspaceOpsRoot via WorkspaceRoot . Looks like we still link to that page https://github.com/dagster-io/dagster/blob/master/js_modules/dagster-ui/packages/ui-core/src/assets/UnderlyingOpsOrGraph.tsx#L29
• https://github.com/dagster-io/dagster/blob/master/js_modules/dagster-ui/packages/ui-core/src/workspace/WorkspaceGraphsRoot.tsx - weird composite solid specific thing? likely safe to delete

edit:

• Also see the usedSolid look-up by name in use here https://github.com/dagster-io/dagster/blob/master/js_modules/dagster-ui/packages/ui-core/src/ops/OpDetailsRoot.tsx#L70

[alangenfeld]

I'm cool with this change if we delete / update the remaining surface of code that depended on this assumption

[schrockn]

Strongly support this change assuming we audit all UI, GraphQL, and Python APIs that rely on this property.

[achen187]

please please please merge this!

[cyberjar09]

does this address situations where assets/ops have the same names across code locations? 🤔

[smackesey]

I had a peek at this. I confirmed that in its current state the PR produces an incorrect result on the ops root page (as flagged by @alangenfeld) for a code location. In the attached image (which uses Sandy's sample code) there should be two instances of op1:

Resolving this issue is a little tricky-- I believe we need to record differentiating information for nodes of the same name when constructing the repository, as there is currently no means of differentiating them on the ExternalJob object. Since we are maintaining unique names within a job, there's no need to touch the OpDefSnap and NodeDefSnap objects to add a unique identifier. I think probably we just need to:

• Assemble a sequence of unique nodes on RepositoryData, where index can be used to identify them.
• Add a field to ExternalJob that maps node names to indices in this unique node sequence.
• Add a uniqueId field to GrapheneUsedSolid that uses this index.
• Add this uniqueId field to the required variables when querying for a single used solid.
• Because the only time we look up individual usedSolid is from the Ops root, we will have the index available because we populated it in the full usedSolids list on that page

I could stack those changes on this PR and then I think it could be merged?

[schrockn]

@smackesey @sryza Just reviving this as I think it is a positive change. If the only page that is affected here is the ops root page, I think we should consider just eliminating the UI screen and the graphql field that backs it, rather than a complicated internal refactor.

[sryza]

Just reviving this as I think it is a positive change. If the only page that is affected here is the ops root page, I think we should consider just eliminating the UI screen and the graphql field that backs it, rather than a complicated internal refactor.

+1.

Alas I don't think I will personally have the bandwidth for this in the near term.

[schrockn]

Just reviving this as I think it is a positive change. If the only page that is affected here is the ops root page, I think we should consider just eliminating the UI screen and the graphql field that backs it, rather than a complicated internal refactor.

+1.

Alas I don't think I will personally have the bandwidth for this in the near term.

Roger. Made a tracking task and will put on foundations backlog.

[salazarm]

From the UI side if we want to do this we'll need unique IDs for the ops

[rd-danny-fleer]

Is there any progress on this PR? Definetly need this cause we're affected by #17190

[sryza]

Alas I have not had a chance to make progress on this. Going to close on that account.