Spack extensions can import code from modules in root or cmd folder by alalazo · Pull Request #11209 · spack/spack

alalazo · 2019-04-17T15:46:03Z

This PR is a follow up of #8612 and an alternative implementation of the improved import capabilities in #11145.

The problem with #8612 was basically that for complex commands one couldn't split source code into python modules that were residing:

In the extension root folder
In a subfolder of the cmd directory

This PR solves that issue by ensuring that, for each extension found, the python packages associated with the root folder and the cmd folder are in sys.modules with an appropriate __path__ set.

List of modifications:

Spack extensions can import code from modules in root or cmd folder (either relative or absolute imports)
Added unit tests to verify this behavior

alalazo · 2019-04-17T16:49:21Z

lib/spack/spack/extensions.py

+    # Create a searchable package for both the root folder of the extension
+    # and the subfolder containing the commands
+    ensure_package_creation(extension)
+    ensure_package_creation(extension + '.cmd')


The two lines above create two python packages named spack.extensions.<extension-name> and spack.extensions.<extension-name>.cmd. This means that:

The __init__.py file in those two extension folders is always executed when Spack imports commands

A user can refer to python modules either in the extension root or in any subfolder of the <ext-root>/cmd directory

What happens if the __init__.py file doesn't exist in either case? Is it created, or treated as if it existed but was empty, or is this an error?

I'm also unsure as to what this does to solve this issue we had with SpackDev in the original implementation of PR #8612, namely that package globals weren't persisted across calls to functions in the package (in Python 2.7). Would it be possible to extend your test to verify behavior in this case? Specifically import a command, call one command to set a global and another to retrieve it, verifying that the value retrieved is as set?

Note that one thing the revised PR #11145 does that is not addressed here is to store the location of commands as they are found to avoid having to try and fail to import up to n-1 times for n configured extension paths.

alalazo · 2019-04-17T16:49:48Z

lib/spack/spack/test/extensions.py

+
+# Test a relative import
+from ..implementation import hello_folks
+


The test here makes sure that both absolute and relative imports work.

alalazo · 2019-04-17T18:05:30Z

@scheibelp @chissg If you are fine with the changes above I'll add docs for the extensions with this PR

scheibelp · 2019-04-17T20:29:54Z

@alalazo I didn't see discussion from #11145 which suggested that it was not salvageable, so I'm curious why you added an alternative implementation.

That being said, this does have tests. So @chissg if it seems suitable for your purposes, it is attractive to me for at least that reason.

alalazo · 2019-04-18T08:27:57Z

I didn't see discussion from #11145 which suggested that it was not salvageable, so I'm curious why you added an alternative implementation.

In the review I recognized the lack of unit tests in #8612, and started this PR to add them. Looking at the code linked in #11145 (comment) it seemed to me that what was missing from #8612 - please @chissg correct me if I'm wrong - was the possibility to import code outside of the cmd folder. So I added a unit test that creates on the fly an extension with the following layout:

spack-testcommand
    └── testcommand
        ├── cmd
        │   └── hello.py
        ├── implementation.py
        └── __init__.py

where the command in hello.py refers both by relative and absolute imports to functions in implementation.py. This unit test was failing with the current develop branch and the proposed solution in #11145 was changing a few APIs in spack/cmd/__init__.py and spack/extensions.py + adding two or three global variables at module scope.

At that point I started reading through the mess that is the import mechanism in Python and figured out a less invasive solution to this based on package path rules, implemented it to check it worked and committed it here.

Besides the fact that there are no API changes and no additional global variables added, I think the biggest difference this PR brings wrt #11145 is that by using package path rules instead of global sys.path like in:

spack/lib/spack/spack/extensions.py

Lines 60 to 75 in 831f829

    
           def _init_extension_command_map(): 
        
               """Return the list of paths where to search for command files.""" 
        
               global _extension_command_map 
        
               if _extension_command_map is None: 
        
                   _extension_command_map = {} 
        
                   extension_paths = spack.config.get('config:extensions') or [] 
        
                   sys.path.extend(extension_paths) 
        
                   for path in extension_paths: 
        
                       extension = extension_name(path) 
        
                       if extension: 
        
                           command_path = os.path.join(path, extension, 'cmd') 
        
                           _command_paths.append(command_path) 
        
                           commands = spack.cmd.find_commands(command_path) 
        
                           _extension_command_map.update( 
        
                               dict((command, path) for command in 
        
                                    commands if command not in _extension_command_map))

we effectively insulate extensions from one other, while having all the paths added in sys.path an extension can possibly refer to another depending on the import order.

alalazo · 2019-04-18T08:30:37Z

@scheibelp To add on the comment above, #11145 improves error handling for missing or mistyped commands - that part is not present in this PR, I just dealt with imports.

greenc-FNAL · 2019-04-18T19:58:54Z

lib/spack/spack/extensions.py

+    # Create a searchable package for both the root folder of the extension
+    # and the subfolder containing the commands
+    ensure_package_creation(extension)
+    ensure_package_creation(extension + '.cmd')


What happens if the __init__.py file doesn't exist in either case? Is it created, or treated as if it existed but was empty, or is this an error?

I'm also unsure as to what this does to solve this issue we had with SpackDev in the original implementation of PR #8612, namely that package globals weren't persisted across calls to functions in the package (in Python 2.7). Would it be possible to extend your test to verify behavior in this case? Specifically import a command, call one command to set a global and another to retrieve it, verifying that the value retrieved is as set?

Note that one thing the revised PR #11145 does that is not addressed here is to store the location of commands as they are found to avoid having to try and fail to import up to n-1 times for n configured extension paths.

alalazo · 2019-04-18T20:15:21Z

What happens if the init.py file doesn't exist in either case? Is it created, or treated as if it existed but was empty, or is this an error?

It's treated as if empty, the code that deals with it is here:

spack/lib/spack/spack/extensions.py

Lines 58 to 63 in 9b1f1ca

    
           parts = [path] + name.split('.') + ['__init__.py'] 
        
           init_file = os.path.join(*parts) 
        
           if os.path.exists(init_file): 
        
               m = llnl.util.lang.load_module_from_file(package_name, init_file) 
        
           else: 
        
               m = types.ModuleType(package_name)

alalazo · 2019-04-18T20:19:28Z

Would it be possible to extend your test to verify behavior in this case? Specifically import a command, call one command to set a global and another to retrieve it, verifying that the value retrieved is as set?

Sure. Can you be more specific on what you mean by "call one command to set a global and another to retrieve it"? Going back to the example above, do you mean something like:

Call from hello.py a function that sets a global in implementation.py
Then call from hello.py a function that retrieves it and check the value

?

greenc-FNAL · 2019-04-18T20:27:14Z

In the specific case I have in mind, dev.py, globals _subcmds and _subcmd_functions are set by a call to setup_parser() and then used by a subsequent call to dev(). In the normal way in which commands are initialized by Spack, I don't actually know whether these calls are made to the same module object, or to two module objects obtained by distinct import commands. Either way, I think it would be important for the test to duplicate this pattern.

alalazo · 2019-04-19T08:31:13Z

@chissg The test case requested should be in d1934f6. One thing that was not clear, at least to me, when reading the API docs is that if you don't save the loaded module in sys.modules global objects won't persist.

* New function spack.cmd.find_commands(), allowing for the location of subcommands. * New function spack.cmd.get_module_from() to allow subcommand module loading from alternative namespaces. * Improve error handling on missing, misspelled or broken command, including as part of `spack help`. If --debug is active, produce a stack trace. * Extension command paths are cached, including which commands are to be found at which path. * Corrected help text for --spec option (drive-by).

lib/spack/spack/test/extensions.py

scheibelp · 2019-05-04T01:37:20Z

In addition to addressing #11209 (comment), could you also rename the extensions test module to cmd_extensions to clarify that it is for testing command extensions rather than Spack extension packages? To be clear, I'm not asking you to rename lib/spack/spack/extensions.py, just lib/spack/spack/test/extensions.py.

alalazo · 2019-05-05T13:53:47Z

In addition to addressing #11209 (comment), could you also rename the extensions test module to cmd_extensions [...] lib/spack/spack/test/extensions.py.

No prob renaming that, but I'd like later to improve this feature to allow for:

Defining hooks in extensions
Defining new configuration files in extensions, if need be
Maybe have a cli interface to help managing external commands etc.

so I'm wondering whether we should look for a more general name than cmd_extensions (or maybe use package_extensions if we add test for extension packages?). In any case, it's fine with me if we want to revisit this discussion later and for the time being just rename the test file to cmd_extensions.py.

scheibelp · 2019-05-06T18:11:07Z

I'd prefer it be named cmd_extensions for now.

I think that name would also be suitable later (even if the functionality you mention in #11209 (comment) is added).

maybe use package_extensions if we add test for extension packages?

Now that there are two types of extensions (extension packages, and command extensions), it would be best to be explicit about both. In the future I think the extensions module in lib/spack/spack should also be renamed for clarity. And if extension packages were tested in their own module in the future (for now I think there are tests but in the Package-testing logic) then it should be called package_extensions.

lib/spack/spack/test/extensions.py

alalazo · 2019-05-11T17:01:57Z

@scheibelp Done!

scheibelp · 2019-05-14T01:18:34Z

lib/spack/spack/extensions.py

-    module_name = '{0}.{1}'.format(__name__, python_name)

    try:
+        # TODO: Upon removal of support for Python 2.6 substitute the call


@alalazo sorry - another question: does this apply to all calls to load_module_from_file, or just this one? If it's just this one, then why is that?

It's just this one. The reason has to do with how the import mechanism works (I had to read carefully through the docs to understand that). This snippet:

spack/lib/spack/spack/extensions.py

Lines 72 to 78 in cdfcf7a

# Setting __path__ to give spack extensions the

# ability to import from their own tree, see:

#

# https://docs.python.org/3/reference/import.html#package-path-rules

#

m.__path__ = [os.path.dirname(init_file)]

sys.modules[package_name] = m

is used to add a search __path__ to both the "root" extension module and its command submodule, e.g. spack.extensions.testcommand and spack.extensions.testcommand.cmd, which are imported here:

spack/lib/spack/spack/extensions.py

Lines 80 to 83 in cdfcf7a

# Create a searchable package for both the root folder of the extension

# and the subfolder containing the commands

ensure_package_creation(extension)

ensure_package_creation(extension + '.cmd')

After we do this, we won't need the absolute path of each command file anymore, as that will be computed from the __path__ of the corresponding subpackage. For instance, if I import spack.extensions.testcommand.cmd.hello the import mechanism will compute the corresponding file path from the first __path__ attribute found going up with subpackages i.e. spack.extensions.testcommand.cmd.

Now, in python 2.6 to import a module based on a computed string we need either to use directly the __import__ function or the imp module - and both approaches are not recommended. From python 2.7 on there's importlib.import_module which seems to be the idiomatic way and the comment above is a reminder that we could just use that instead of loading the module and adding it to sys.modules.

@scheibelp Let me know if something should be clarified in the comment above or if it's fine like that. Thanks!

scheibelp · 2019-05-17T00:27:48Z

Thanks!

* New function spack.cmd.find_commands(), allowing for the location of subcommands. * New function spack.cmd.get_module_from() to allow subcommand module loading from alternative namespaces. * Improve error handling on missing, misspelled or broken command, including as part of `spack help`. If --debug is active, produce a stack trace. * Extension command paths are cached, including which commands are to be found at which path. * Corrected help text for --spec option (drive-by).

* New function spack.cmd.find_commands(), allowing for the location of subcommands. * New function spack.cmd.get_module_from() to allow subcommand module loading from alternative namespaces. * Improve error handling on missing, misspelled or broken command, including as part of `spack help`. If --debug is active, produce a stack trace. * Extension command paths are cached, including which commands are to be found at which path.

alalazo added feature A feature is missing in Spack tests General test capability(ies) extensions commands don't-merge-yet labels Apr 17, 2019

alalazo requested review from greenc-FNAL and scheibelp April 17, 2019 16:44

alalazo commented Apr 17, 2019

View reviewed changes

greenc-FNAL requested changes Apr 18, 2019

View reviewed changes

alalazo added documentation Improvements or additions to documentation and removed don't-merge-yet labels Apr 19, 2019

greenc-FNAL approved these changes Apr 22, 2019

View reviewed changes

scheibelp reviewed May 4, 2019

View reviewed changes

lib/spack/spack/test/extensions.py Outdated Show resolved Hide resolved

alalazo force-pushed the features/docs_and_test_for_extensions branch from b0fb212 to b0dc9be Compare May 5, 2019 13:05

scheibelp reviewed May 6, 2019

View reviewed changes

lib/spack/spack/test/extensions.py Outdated Show resolved Hide resolved

alalazo added 2 commits May 11, 2019 16:38

Spack extensions can import code from modules in root or cmd folder

875cb10

Added unit tests for Spack extensions

2f4086b

alalazo added 4 commits May 11, 2019 16:38

Fixed setting globals in command modules + added test to check behavior

89720b0

Added minimal docs for extensions

c8b4889

Better cleanup of Spack extensions unit tests

73ba58f

Renamed test function and file

cdfcf7a

alalazo force-pushed the features/docs_and_test_for_extensions branch from b0dc9be to cdfcf7a Compare May 11, 2019 15:29

scheibelp reviewed May 14, 2019

View reviewed changes

alalazo mentioned this pull request May 16, 2019

Adding singularity command group with recipe|build #11367

Closed

scheibelp merged commit c03be0d into spack:develop May 17, 2019

alalazo deleted the features/docs_and_test_for_extensions branch May 17, 2019 03:15

greenc-FNAL mentioned this pull request May 22, 2019

[WIP] Generalize command-finding to support sub-commands, incl. for extensions. #11145

Closed


		# Test a relative import
		from ..implementation import hello_folks

	# Setting __path__ to give spack extensions the
	# ability to import from their own tree, see:
	#
	# https://docs.python.org/3/reference/import.html#package-path-rules
	#
	m.__path__ = [os.path.dirname(init_file)]
	sys.modules[package_name] = m

	# Create a searchable package for both the root folder of the extension
	# and the subfolder containing the commands
	ensure_package_creation(extension)
	ensure_package_creation(extension + '.cmd')

Conversation

alalazo commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

greenc-FNAL Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

alalazo commented Apr 17, 2019

Uh oh!

scheibelp commented Apr 17, 2019

Uh oh!

alalazo commented Apr 18, 2019

Uh oh!

alalazo commented Apr 18, 2019

Uh oh!

greenc-FNAL Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo commented Apr 18, 2019

Uh oh!

alalazo commented Apr 18, 2019

Uh oh!

greenc-FNAL commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Apr 19, 2019

Uh oh!

Uh oh!

scheibelp commented May 4, 2019

Uh oh!

alalazo commented May 5, 2019

Uh oh!

scheibelp commented May 6, 2019

Uh oh!

Uh oh!

alalazo commented May 11, 2019

Uh oh!

scheibelp May 14, 2019

Choose a reason for hiding this comment

Uh oh!

alalazo May 15, 2019

Choose a reason for hiding this comment

Uh oh!

alalazo May 16, 2019

Choose a reason for hiding this comment

Uh oh!

scheibelp commented May 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alalazo commented Apr 17, 2019 •

edited

Loading

greenc-FNAL Apr 18, 2019 •

edited

Loading

greenc-FNAL Apr 18, 2019 •

edited

Loading

greenc-FNAL commented Apr 18, 2019 •

edited

Loading