Page MenuHomePhabricator

Resolves https://github.com/dagster-io/dagster/issues/1913
AbandonedPublic

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

Details

Summary

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

Unit

Diff Detail

Repository
R1 dagster
Branch
propitiating-the-dark-forces
Lint
Lint OK
Unit
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

python_modules/dagster/dagster/core/definitions/handle.py
162–167

/me googles apotropaically

184–187

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?

max abandoned this revision.Apr 8 2020, 9:42 PM