Page MenuHomePhabricator

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

Authored by max on Sat, Nov 16, 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.Sat, Nov 16, 5:20 AM
nate added a subscriber: nate.Sun, Nov 17, 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
150–155

/me googles apotropaically

172–175

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

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

Test coverage

this is really scary to me

max removed a reviewer: Restricted Project.Tue, Nov 19, 2:55 AM
schrockn requested changes to this revision.Wed, Nov 20, 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.Wed, Nov 20, 7:44 PM
max added a comment.Mon, Nov 25, 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?