Page MenuHomePhabricator

Check that all definition names are invalid and warn
ClosedPublic

Authored by schrockn on Sep 9 2020, 12:44 PM.

Details

Summary

Because we're good at stuff we were actually never checking that the pipeline name
as well as many other definitions was valid. This change makes that warn
if there is an invalid name. We'll break after we push next week.

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 valid name to RFC: Check that pipeline is valid.
schrockn edited the summary of this revision. (Show Details)
schrockn added reviewers: prha, alangenfeld.

up

Given the breaking changes angle on this, I think we should try to hit all the defs that we missed (everything except solid ๐Ÿฅด ?) in one go

Also lets include the CHANGES.md entry while we are here since there some subtlety to it and I dont thing rel-eng should have to figure it out last minute

to your queue for thoughts above - and changed.md

This revision now requires changes to proceed.Sep 10 2020, 4:00 PM

yup. should we do a warning for a bit?

schrockn retitled this revision from RFC: Check that pipeline is valid to RFC: Check that all definition names are invalid; warn on pipeline.
schrockn edited the summary of this revision. (Show Details)

up

schrockn retitled this revision from RFC: Check that all definition names are invalid; warn on pipeline to RFC: Check that all definition names are invalid and warn.
schrockn edited the summary of this revision. (Show Details)

up

python_modules/dagster/dagster/core/definitions/hook.py
23

this one seems fine to "break" since its experimental

python_modules/dagster/dagster/core/definitions/preset.py
54

should this be a warn too ?

python_modules/dagster/dagster/core/definitions/reconstructable.py
25โ€“28

hmmmmm, maybe a prefix like _R_ or something? pipeline__ alone as a prefix doesn't feel great to me

dgibson added inline comments.
python_modules/dagster/dagster/core/definitions/pipeline.py
155

should this be a const if it affects business logic

python_modules/dagster/dagster/core/definitions/utils.py
74

update version

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_preset_definition.py
179

maybe something that looks less like a typo :)

This revision is now accepted and ready to land.Tue, Sep 29, 4:00 PM
schrockn retitled this revision from RFC: Check that all definition names are invalid and warn to Check that all definition names are invalid and warn.
schrockn removed a reviewer: dgibson.

up

This revision now requires review to proceed.Wed, Sep 30, 10:46 PM
python_modules/dagster/dagster/core/definitions/pipeline.py
155

probably, but we do it elsewhere so seems like a separate diff

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Sep 30, 11:25 PM
This revision was automatically updated to reflect the committed changes.