Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

luigi: enable local modules discovery #51901

Merged
merged 1 commit into from Dec 19, 2018
Merged

Conversation

yrashk
Copy link
Contributor

@yrashk yrashk commented Dec 12, 2018

Currently, local module discovery is broken with luigi installed from
this derivation.

In the documentation, you can see the following command line syntax
used:

luigi --module module_name ...

However, currently this will result in an error:

ModuleNotFoundError: No module named 'module_name'

However, if the call is prepended with this:

PYTHONPATH=.:$PYTHONPATH

then it will work as expected.

This patch makes this the default behaviour.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Currently, local module discovery is broken with luigi installed from
this derivation.

In the documentation, you can see the following command line syntax
used:

```
luigi --module module_name ...
```

However, currently this will result in an error:

```
ModuleNotFoundError: No module named 'module_name'
```

However, if the call is prepended with this:

```
PYTHONPATH=.:$PYTHONPATH
```

then it will work as expected.

This patch makes this the default behaviour.
@matthewbauer
Copy link
Member

/cc @bhipple

@Mic92
Copy link
Member

Mic92 commented Dec 18, 2018

@GrahamcOfBorg build luigi

@@ -19,6 +19,9 @@ python3Packages.buildPythonApplication rec {
# Requires tox, hadoop, and google cloud
doCheck = false;

# This enables accessing modules stored in cwd
makeWrapperArgs = ["--prefix PYTHONPATH . :"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first glance this approach seems a little impure. If you're testing user-provided python modules in dev shouldn't you set PYTHONPATH on your own to your own modules, rather than have all invocations of luigi add ., wherever that may be? Once you have your module ready to go to production, it seems like the best way might be to package it properly and add it as a dependency of luigi (perhaps via an extra plugins attribute), as we do for things like git plugins?

@yrashk
Copy link
Contributor Author

yrashk commented Dec 18, 2018 via email

@bhipple
Copy link
Contributor

bhipple commented Dec 18, 2018

Indeed, one of the greatest strengths of NixPkg is that things tend to "just
work", because the higher level of purity allows us to test more faithfully and
reproduce environments consistently across machines -- so I'm all for changes
that make the out-of-the-box experience more seamless.

In this particular case, though, it appears that the command does not "just
work" for the official examples, either, and fails in this same way:

Given that the upstream docs tell users to set PYTHONPATH themselves when
working with local modules and examples, it seems like we should stay consistent
with them? Even if we make no code changes I think we should at least leave a
comment in the luigi/defalt.nix explaining how to load your own user modules.

Coincidentally, I'm working on a very similar PR (#51597), but the contrast
there is that the binary always depends on those modules at runtime, and those
modules are other NixPkgs. (This'll eventually be fixed by #44047.)

@Mic92
Copy link
Member

Mic92 commented Dec 18, 2018

The difference might be however that PYTHONPATH in our case would always contain a value, making the upstream documentation invalid (i.e. PYTHONPATH="" ... has no effect).

@yrashk
Copy link
Contributor Author

yrashk commented Dec 18, 2018

Those are very good references, but I am here because of https://luigi.readthedocs.io/en/stable/running_luigi.html :) It does not mention the need for the parameter.

Anyway, not a big deal. Perhaps their documentation should be fixed in the first place.

@bhipple
Copy link
Contributor

bhipple commented Dec 19, 2018

Ah, seems like this is the standard way to invoke luigi; in that case this seems fine (and possibly even preferable). There's a brief comment in that list saying that sys.path has to have my_module in it, but doing that kind of thing in NixOS can be challenging for beginners.

BTW, feel free to add yourself to the maintainers list for this package if you're interested in helping to update, test, and improve it -- there's a good chance you know more about luigi than I do! (No pressure if you're not inclined.)

@Mic92 Mic92 merged commit cd906a3 into NixOS:master Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants