Page MenuHomePhabricator

Needs RevisionPublic

Authored by max on Nov 16 2019, 5:20 AM.



This eliminates the very unpleasant ImportError: attempted relative import with no known parent package which, horribly, in the status quo ante appeared only when targeting a file for execution (e.g., dagit -f -n) that was in fact doing valid relative imports in a valid package. I suspect our users also hit this error and, like most of us most of the time, don't understand that it means they should be using dagit -m instead.

Test Plan


Diff Detail

R1 dagster
Lint OK
No Unit Test Coverage

Event Timeline

max created this revision.Nov 16 2019, 5:20 AM
nate added a subscriber: nate.Nov 17 2019, 7:26 PM

Is there any way to construct some tests that showcase expected behaviors under the different scenarios here? I'm a little nervous about ensuring the different logic paths are correct

this is a lot to take in


/me googles apotropaically


we only want this to happen if we added the dir right?

max updated this revision to Diff 6690.Nov 18 2019, 9:36 PM

Test coverage

this is really scary to me

max removed a reviewer: Restricted Project.Nov 19 2019, 2:55 AM
schrockn requested changes to this revision.Nov 20 2019, 7:44 PM

I'd like us to consider a more constrained approach, where we disallow loading from a file in a module context. I think with a good error message this is fine. (We could even print out the exact command.) This imposes the requirement on the user that they have the module installed, but I believe this is reasonable requirement that would dramatically simplify things.

This revision now requires changes to proceed.Nov 20 2019, 7:44 PM
max added a comment.Nov 25 2019, 6:37 PM

I really think the requested changes would make the local dev experience much more onerous -- essentially, we're saying to users, either make your local dev env pip installable, or edit your python path manually every time you want to work on something new. FWIW, I am probably going to keep this changeset around locally for my own convenience, even if we don't land it.

Adding @themissinghlink because he hit this same issue locally and concluded that the CLI tools didn't work.

I'm a bit confused re: the comment on altering the path. From my perspective what I suggesting supports two use cases:

  1. Loading by module if the code is in a module.
  2. Loading by file if the code is in a bare file.

If you are trying to load by file in a module context, it would clearly communicate an error (perhaps even with the exact command to launch in the error message).

Where do you have to alter the path?