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
Conversation
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.
/cc @bhipple |
@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 . :"]; |
There was a problem hiding this comment.
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?
Your point makes sense. However, the experience of an oblivious newcomer is
that one of "copy pasting command line examples from the official
documentation" doesn't work. What's a worse trade-off?
Can we expose the impure version by default and allow the previous version
to be enabled with a flag, for example?
…On Tue, Dec 18, 2018, 8:15 PM Benjamin Hipple ***@***.*** wrote:
***@***.**** commented on this pull request.
------------------------------
In pkgs/applications/networking/cluster/luigi/default.nix
<#51901 (comment)>:
> @@ -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 . :"];
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?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#51901 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAABxHTpy3WhWtXKmbm_1LsdqRXL19Lzks5u6OqNgaJpZM4ZPBdO>
.
|
Indeed, one of the greatest strengths of NixPkg is that things tend to "just In this particular case, though, it appears that the command does not "just
Given that the upstream docs tell users to set Coincidentally, I'm working on a very similar PR (#51597), but the contrast |
The difference might be however that |
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. |
Ah, seems like this is the standard way to invoke 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.) |
Currently, local module discovery is broken with luigi installed from
this derivation.
In the documentation, you can see the following command line syntax
used:
However, currently this will result in an error:
However, if the call is prepended with this:
then it will work as expected.
This patch makes this the default behaviour.
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)