Page MenuHomeElementl

NodeHandle serdes adjustments
ClosedPublic

Authored by alangenfeld on Jul 16 2021, 7:44 PM.

Details

Summary

for better cross version compat - continue to use "SolidHandle" as the serialized name for NodeHandle

Test Plan

added test

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This comment has been deleted.
python_modules/dagster/dagster/core/definitions/dependency.py
230

not sure I fully understand the implications of this.

If we serialized these with the NodeHandle name, is the problem that old pipeline snapshots would not be possible to deserialize? If that's the case, will this special case serializer need to be around forever?

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

So [1] handles the loading old SolidHandle strings in to NodeHandle

the problem this diff addresses is *old* code trying to load the something serialized by the *new* code. Since NodeHandle doesn't exist in old, it will fail if it trys to load something serialized by new.

This shouldn't really happen ideally, I think it will be reasonable for us to expect dagit is as new as the newest user code deployment for example

but there are some current set-ups we have that don't abide by this so im just being extra generous with the back compat.

will this special case serializer need to be around forever?

I think something like [1] we will keep around for a long time - but this serializer we can potentially remove if we better enforce the scheme described above

386–387

[1]

owen added inline comments.
python_modules/dagster/dagster/core/definitions/dependency.py
230

ah perfect -- I didn't realize that a prior diff had touched stuff in this area already. in that case, looks good to me.

This revision is now accepted and ready to land.Jul 16 2021, 10:30 PM
This revision was automatically updated to reflect the committed changes.