Skip to content

Conversation

@rjleveque
Copy link
Member

This seems to be the correct way to import a module based on its path, if it's not part of clawpack or some other package, but it has enough steps that it seems worthwhile having a function to do it. I often want to do this since I have various utility modules that I use in many different projects.

Suggestions welcome if others know more about the best way to do this.

Copy link
Member

@mandli mandli left a comment

Choose a reason for hiding this comment

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

I think this will be nice to have! There were a couple of thoughts I had, but I think the only question I would have is what would happen if say topotools was already imported and you called fullpath_import('/path/to/my/topotools.py')? I don't think it will import the new one, which may be confusing.

@rjleveque
Copy link
Member Author

@mandli and I played around with this a bit and it seems to work fairly robustly, although we don't really understand how sys.modules is used.

I did a test where I copied topotools.py to my home directory and modified it to just add print('my modified topotools') to the module and observe the following:

In [55]: my_topotools = fullpath_import('/Users/rjl/topotools.py')
my modified topotools
loaded module from file:  /Users/rjl/topotools.py

In [56]: sys.modules['topotools']
Out[56]: <module 'topotools' from '/Users/rjl/topotools.py'>

In [57]: from clawpack.geoclaw import topotools

In [58]: topotools
Out[58]: <module 'clawpack.geoclaw.topotools' from '/Users/rjl/git/clawpack/geoclaw/src/python/geoclaw/topotools.py'>

In [59]: sys.modules['topotools']
Out[59]: <module 'topotools' from '/Users/rjl/topotools.py'>

In [60]: reload(topotools)
Out[60]: <module 'clawpack.geoclaw.topotools' from '/Users/rjl/git/clawpack/geoclaw/src/python/geoclaw/topotools.py'>

Note that importlib.reload reloads topotools from geoclaw (as one would hope) even though sys.modules['topotools'] points to my modified version.

Also, as expected, reload cannot be used to reload my_topotools, although the error message it gives is a bit mysterious:

In [61]: reload(my_topotools)
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
Cell In[61], line 1
----> 1 reload(my_topotools)

File /opt/homebrew/Cellar/[email protected]/3.13.3_1/Frameworks/Python.framework/Versions/3.13/lib/python3.13/importlib/__init__.py:128, in reload(module)
    126 spec = module.__spec__ = _bootstrap._find_spec(name, pkgpath, target)
    127 if spec is None:
--> 128     raise ModuleNotFoundError(f"spec not found for the module {name!r}", name=name)
    129 _bootstrap._exec(spec, module)
    130 # The module may have replaced itself in sys.modules!

ModuleNotFoundError: spec not found for the module 'topotools'

Reloading by using fullpath_import again works fine after chaning the print statement:

In [62]: my_topotools = fullpath_import('/Users/rjl/topotools.py')
my modified topotools -- version 2
loaded module from file:  /Users/rjl/topotools.py

@mandli
Copy link
Member

mandli commented Sep 6, 2025

I should have pointed to this earlier, but could we use this function instead of what is in the test.py in the new proposed tests in classic clawpack/classic#96?

@ketch
Copy link
Member

ketch commented Sep 6, 2025

Wow, this has actually uncovered a significant bug! One that has been in my code probably since I wrote it back during my PhD. Not sure how this didn't cause problems before.

It's a rather annoying issue: in SharpClaw, there are different options for limiting -- you can limit based on waves, or based on conserved quantities. But that means that the size of the mthlim array should sometimes be num_waves and sometimes num_eqn.

I guess the right thing would be to make this array get resized based on which kind of limiting is selected, but that seems awkward and the truth is we almost never use the flexibility of applying different limiters to different fields. So I'm tempted to just say that if you limit conserved quantities, then you can only choose one limiter (i.e. mthlim(1)) and it will get applied to all of them. What do you think @mandli?

@mandli
Copy link
Member

mandli commented Sep 7, 2025

Wow, this has actually uncovered a significant bug! One that has been in my code probably since I wrote it back during my PhD. Not sure how this didn't cause problems before.

It's a rather annoying issue: in SharpClaw, there are different options for limiting -- you can limit based on waves, or based on conserved quantities. But that means that the size of the mthlim array should sometimes be num_waves and sometimes num_eqn.

I guess the right thing would be to make this array get resized based on which kind of limiting is selected, but that seems awkward and the truth is we almost never use the flexibility of applying different limiters to different fields. So I'm tempted to just say that if you limit conserved quantities, then you can only choose one limiter (i.e. mthlim(1)) and it will get applied to all of them. What do you think @mandli?

I think you meant this for clawpack/pyclaw#748?

@ketch
Copy link
Member

ketch commented Sep 7, 2025

I think you meant this for clawpack/pyclaw#748?

Yes, you're right. I'll move the comment there.

@rjleveque
Copy link
Member Author

I should have pointed to this earlier, but could we use this function instead of what is in the test.py in the new proposed tests in classic clawpack/classic#96?

@mandli: I think you are referring to the way we need to make sure that import setrun gets the setrun.py from the example currently being tested, rather than reusing a previously imorted one? Yes, I think this should work for that purpose.

@rjleveque
Copy link
Member Author

I made the change suggested by @mandli to use pathlib.Path in place of os.path, which now allows the input fullpath to be a Path object or a string.

Also, if it is a string that starts with $, then the path is now assumed to start with an environment variable and this is resolved, if possible, from os.environ. For example, this works:

    setrun_file = '$CLAW/amrclaw/examples/advection_2d_swirl/setrun.py'
    setrun = util.fullpath_import(setrun_file)

Slightly simpler than having to evaluate os.environ to set fullpath yourself, and it should be useful for people like me who use environment variables to refer to the top level directories of many different projects, which change on different computers or for collaborators.

@mandli
Copy link
Member

mandli commented Sep 8, 2025

I searched around a bit for something like expandvars and found this gist that may provide a simpler way to do the expansion.

I am a bit torn as to whether to suggest that we include something that would expand environment variables as I can see the argument for not doing this without explicitly doing so by the user. That being said, the security and odd path strings that would come up are probably minimal as long as we do not use this in code that will be executed often.

@rjleveque
Copy link
Member Author

Thanks @mandli, it's now much cleaner and also handles ~ and ~user constructs and resolves symlinks.

@mandli
Copy link
Member

mandli commented Sep 9, 2025

Last thing, may want to switch the default verbose to False.

@mandli mandli merged commit 336e422 into clawpack:master Sep 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants