Legion_python -m can not find the module

[eddy16112]

I have a sub-module jupyter under legate module, so the file directory is legate/jupyter/__main__.py.
If I run

legate -m legate.jupyter

then legion_python can not find the module.
If I run

legate -m jupyter

then it works.

I think the issue comes from here, it tries to find the file named legate.jupyter/__main__.py
https://github.com/StanfordLegion/legion/blob/master/bindings/python/legion_top.py#L378-L382

[lightsighter]

@elliottslaughter Do we need to do something more general then just translate the nested module string into a directory structure? Does Python require that all submodules live in nested directories (I think it does)?

[elliottslaughter]

What about Python Eggs? Maybe we don't care—and I'm fine with writing this off—but I'm pretty sure those break these assumptions.

But otherwise yes, that is my understanding.

[lightsighter]

As far as I understand it, eggs have been replaced by wheels. @bryevdv does that sound right for you? Can you think of any cases where we might need to support eggs for legate/cunumeric?

[bryevdv]

I can't think of one. Here is a relevant observation in an issue that proposes deprecating egg uploads on warehouse

As of Dec 2021, eggs have accounted for less than 1% of built distribution uploads.

I haven't personally run into an egg in 10+ years, but if there was any group that might land in that 1% tail I guess it wouldn't surprise me to learn it was HPC users somehow.

[lightsighter]

@eddy16112 Try with this patch and see if it works:

diff --git a/bindings/python/legion_top.py b/bindings/python/legion_top.py
index 6e82a0f8f..128175067 100644
--- a/bindings/python/legion_top.py
+++ b/bindings/python/legion_top.py
@@ -371,7 +371,7 @@ def legion_python_main(raw_args, user_data, proc):
     elif args[start] == '-m':
         if len(args) > (start+1):
             mod_name = args[start+1]
-            filename = mod_name + '.py'
+            filename = os.path.join(*mod_name.split('.')) + '.py'
             found = False
             for path in sys.path:
                 for root,dirs,files in os.walk(path):

[eddy16112]

@lightsighter No, it does not work for me. Here is my fix, let me know if there is any issue with my fix

diff --git a/bindings/python/legion_top.py b/bindings/python/legion_top.py
index 498219304..4ba5476c3 100644
--- a/bindings/python/legion_top.py
+++ b/bindings/python/legion_top.py
@@ -410,9 +410,16 @@ def legion_python_main(raw_args, user_data, proc):
     elif args[start] == '-m':
         if len(args) > (start+1):
             mod_name = args[start+1]
+            mod_dir = mod_name.split(".")
+            if len(mod_dir) > 1:
+                prefix_path = os.path.join(*mod_dir[0:-1])
+                mod_name = mod_dir[-1]
+            else:
+                prefix_path = ""
             filename = mod_name + '.py'
             found = False
             for path in sys.path:
+                path = os.path.join(path, prefix_path)
                 for root,dirs,files in os.walk(path):
                     if mod_name in dirs:
                         main_py = os.path.join(root, mod_name, '__main__.py')

[lightsighter]

It looks fine to me. Can you put that in a branch and let it run through CI while we wait for @elliottslaughter to comment?

[elliottslaughter]

A couple comments:

Isn't mod_dir[0:-1] the same as mod_dir[:-1]? I think the latter is more idiomatic.

I know it's not technically part of what's being modified here, but the use of os.walk is really odd here. Normally, os.walk recurses down into the directory tree. I don't think that's the behavior we want here? Does sys.path implicitly include all recursively contained subdirectories under those explicitly named? The code structure here makes it hard to tell whether we are doing this recursion or not; I see a break at the bottom of the loop body, but I also see several continues that make it look like somtimes it may keep going.

Beyond that the change looks fine to me and I think we can take it as long as it passes CI.

[eddy16112]

sys.path contains directories in PYTHONPATH, but not subdirectories, so that's why we need os.walk to walk through all subdirectories recursively.

[elliottslaughter]

I'm still confused.

Suppose PYTHONPATH=/asdf and we're told to load module a.b.c. Python parses PYTHONPATH into a sys.path value of ["/asdf"]. We iterate this list, get "/asdf". Now we need to check for two (and only two) possible files: /asdf/a/b/c.py and /asdf/a/b/c/__main__.py. This can be accomplished with os.path.exists or similar, and does not require recursive search (or even listing the directory).

If there happens be be a /asdf/qwer/a/b/c.py, it would not be correct to load this (if I understand the semantics of PYTHONPATH correctly). Recursive search is not implied.

Am I missing something?

[eddy16112]

I thought /asdf/qwer/a/b/c.py could be loaded correctly. I looked though the history of this file, looks like os.walk was already there when the python binding was implemented. @lightsighter do you think if this os.walk is a bug https://github.com/StanfordLegion/legion/blob/master/bindings/python/legion_top.py#L377 ? If yes, I am going to fix it by taking @elliottslaughter 's solution.

[lightsighter]

Go ahead and take the new solution as long as it passes all our tests and can still run the cuNumeric test suite. I think I was using os.walk to approximate directory search without looking at the Python path, but it was more than two years ago when I wrote that code so I don't remember exactly. If we have a better solution then we should use it.

[eddy16112]

Here is the fix, will wait for the CI results.
https://gitlab.com/StanfordLegion/legion/-/commit/6176e8bcdb1b7f8abd5e378f6784311d9472076c

[elliottslaughter]

I ran a test to confirm this is Python's expected behavior:

$ mkdir -p asdf/qwer/a/b
$ touch asdf/qwer/a/b/c.py
$ PYTHONPATH=$PWD/asdf python3 -m a.b.c
/Library/Developer/CommandLineTools/usr/bin/python3: Error while finding module specification for 'a.b.c' (ModuleNotFoundError: No module named 'a')
$ PYTHONPATH=$PWD/asdf/qwer python3 -m a.b.c

So yes, this looks like the correct fix to me.

[eddy16112]

The CI is passed, and I have updated the fixes into the master.

[lightsighter]

@eddy16112 Did you test with the cuNumeric test suite?

[eddy16112]

@eddy16112 Did you test with the cuNumeric test suite?

Yes, but I do not think cuNumeric tests -m.

[lightsighter]

Ok, wasn't sure if we used it for any of the test suite execution. I think some of the test suite might use the -m feature, but I am not sure.

[elliottslaughter]

We do not seem to have any Python tests for -m in Legion. Maybe we should add some?

[eddy16112]

We can test pygion tests as -m examples.hello, but we are missing the coverage for __main__.py.

[elliottslaughter]

I have tests for this in the test suite now. I will close once CI passes.

[elliottslaughter]

Tests broke, so I pulled the commit back out and put it in a branch until I figure out what's wrong. Hopefully just missing a path or something.

[eddy16112]

@elliottslaughter I pushed a fix for your tests. https://gitlab.com/StanfordLegion/legion/-/commit/3dd3170e768c968b8fc88299cf88349e5228876a
It should work now.

[elliottslaughter]

Nice, thanks!

It passed CI so I merged to master.

[elliottslaughter]

CI looks good on master.

I think that's it from my end.

[eddy16112]

Close it as the CI is passed.