Page MenuHomePhabricator

[#2220-3] rename runtime_type => dagster_type outside core.definition - args and callsites
ClosedPublic

Authored by yuhan on Mar 5 2020, 11:09 PM.

Details

Summary

Issue: https://github.com/dagster-io/dagster/issues/2220
Deprecated the following args and their call sites

Warings:

  • runtime_type => dagster_type in StepInput and StepOutput

Breaking changes:

  • runtime_type => dagster_type in IntermediateStore.[set|get]_[object|value]

^ Most IntermediateStore.[set/get]_[object/value] are called without argument names. So I decided to rename it without keeping the old name, as there may be fewer breaking changes when just renaming the arg.

Test Plan
  • grep: the only runtime_type stuff left in the codebase is graphql schema related

https://dagster.phacility.com/P37

  • pytest
  • ran example pipelines in dagit
  • 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

yuhan created this revision.Mar 5 2020, 11:09 PM
yuhan retitled this revision from [#2220-3] rename runtime_type => dagster_type outside core.definition - args Issue: https://github.com/dagster-io/dagster/issues/2220 Argument Deprecation Warings: * `runtime_type` => `dagster_type` in `StepInput` and `StepOutput` Breaking... to [#2220-3] rename runtime_type => dagster_type outside core.definition - args.Mar 5 2020, 11:11 PM
yuhan edited the summary of this revision. (Show Details)
yuhan edited the summary of this revision. (Show Details)Mar 5 2020, 11:13 PM
yuhan edited the test plan for this revision. (Show Details)
yuhan added inline comments.Mar 5 2020, 11:23 PM
python_modules/dagster/dagster/core/execution/plan/objects.py
179–192

shouldve not changed the order. this broke many tests. fixing...

yuhan updated this revision to Diff 10314.Mar 5 2020, 11:46 PM
yuhan edited the summary of this revision. (Show Details)

update

yuhan added a comment.EditedMar 6 2020, 12:02 AM

BK fails on lint bc

************* Module dagster_aws_tests.s3_tests.test_intermediate_store
python_modules/libraries/dagster-aws/dagster_aws_tests/s3_tests/test_intermediate_store.py:173:4: W0221: Parameters differ from overridden 'set_object' method (arguments-differ)
************* Module dagster_gcp_tests.gcs_tests.test_intermediate_store
python_modules/libraries/dagster-gcp/dagster_gcp_tests/gcs_tests/test_intermediate_store.py:169:4: W0221: Parameters differ from overridden 'set_object' method (arguments-differ)

the fix is in https://dagster.phacility.com/D2205
https://dagster.phacility.com/D2205#change-niy0j3QFVkwN

fixed^
squashed D2205 into this diff

yuhan edited the summary of this revision. (Show Details)Mar 6 2020, 12:27 AM
yuhan edited the test plan for this revision. (Show Details)
prha accepted this revision.Mar 6 2020, 1:47 AM

This looks good!

Can you create an issue to remove all the backcompat stuff with an 0.8.0 label?

python_modules/dagster/dagster/core/execution/plan/objects.py
197–200

Can probably just do the inst check once... and it might be more accurate to do it according to the passed param.

This revision is now accepted and ready to land.Mar 6 2020, 1:47 AM
yuhan updated this revision to Diff 10335.Mar 6 2020, 6:20 PM
yuhan edited the test plan for this revision. (Show Details)

update more renaming in dagster-graphql

yuhan retitled this revision from [#2220-3] rename runtime_type => dagster_type outside core.definition - args to [#2220-3] rename runtime_type => dagster_type outside core.definition - args and callsites.Mar 6 2020, 10:20 PM
yuhan edited the test plan for this revision. (Show Details)

Follow up issue before next major release: https://github.com/dagster-io/dagster/issues/2246

python_modules/dagster/dagster/core/execution/plan/objects.py
197–200

Did you mean to change it to

runtime_type=check.inst_param(runtime_type, 'dagster_type', DagsterType),  
dagster_type=check.inst_param(dagster_type, 'dagster_type', DagsterType),

or do canonicalize_dagster_type = check.inst_param(canonicalize_dagster_type, 'dagster_type', DagsterType) once before __new__ and assign canonicalize_dagster_type to both params in new?

The former is the same as the current one. The latter would save one check but doesn't really matter IMO. I would prefer the current one if that's the case because it's cleaner when we do the cleanup before the major release. But lemme know if I misunderstood it :)

yuhan requested review of this revision.Mar 6 2020, 10:28 PM

added more renaming in dagster-graphql. also tested through more flows:

prha accepted this revision.Mar 6 2020, 10:38 PM
prha added inline comments.
python_modules/dagster/dagster/core/execution/plan/objects.py
197–200

was just saying that it's weird that the param name for both is dagster_type, even though the passed param name would be runtime_type. It indicates that there will be a misleading error message in the case that the check gets triggered.

This revision is now accepted and ready to land.Mar 6 2020, 10:38 PM
yuhan marked an inline comment as done.Mar 6 2020, 10:52 PM
yuhan added inline comments.
python_modules/dagster/dagster/core/execution/plan/objects.py
197–200

oh makes sense! updated.

yuhan updated this revision to Diff 10344.Mar 6 2020, 10:52 PM
yuhan marked an inline comment as done.

update