-
Notifications
You must be signed in to change notification settings - Fork 230
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
Make and use a julia system image #2443
Changes from all commits
b178895
7c005ff
ca36b62
6110a5d
cbc52e8
613fb28
c616741
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,25 +35,38 @@ | |
import sys | ||
import logging | ||
import itertools | ||
import os | ||
from rmgpy import get_path | ||
|
||
try: | ||
from julia.api import Julia | ||
system_image_path = os.path.join(os.path.dirname(get_path()), "rms.so") | ||
# Should be RMG-Py/rms.so for a normal install, alongside rmg.py | ||
if os.path.exists(system_image_path): | ||
print(f"Using system Julia system image at {system_image_path}") | ||
jl = Julia(sysimage=system_image_path) | ||
else: | ||
print(f"Couldn't find Julia system image at {system_image_path}. " | ||
"Using default Julia system image.") | ||
if __debug__: | ||
# This is the normal case (__debug__ is True by default) | ||
jl = Julia(compiled_modules=False) # Disable incremental precompilation of modules. | ||
else: | ||
# This means that python was run with the -O flag. | ||
# I don't know why or how often that is done | ||
# (all it does usually is remove assert statements), | ||
# nor why one would then need or wish to skip creating a Julia runtime, | ||
# and allow incremental precompilation to be its default, | ||
# but for now I'm trying to refactor without changing behaviour. | ||
pass | ||
from pyrms import rms | ||
from diffeqpy import de | ||
from julia import Main | ||
except Exception as e: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bare exceptions are also considered bad practice. We should try to catch specific exceptions - failing to import Julia api could be because Julia is not installed, failed to load Julia with the image, specific imports fail, etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree in principle, bare exceptions are a code smell. But I'm hoping that in future this whole thing can go away. I think it was introduced so ARC could pass nose tests or something - where this file is imported somewhere but nothing in it needs to actually run. |
||
logging.exception(e) | ||
logging.warning("Could not import pyrms, RMS simulations will not be available") | ||
pass | ||
|
||
if __debug__: | ||
try: | ||
from os.path import dirname, abspath, join, exists | ||
path_rms = dirname(dirname(dirname(abspath(__file__)))) | ||
from julia.api import Julia | ||
jl = Julia(sysimage=join(path_rms,"rms.so")) if exists(join(path_rms,"rms.so")) else Julia(compiled_modules=False) | ||
from pyrms import rms | ||
from diffeqpy import de | ||
from julia import Main | ||
except: | ||
pass | ||
else: | ||
try: | ||
from pyrms import rms | ||
from diffeqpy import de | ||
from julia import Main | ||
except: | ||
pass | ||
|
||
from rmgpy.species import Species | ||
from rmgpy.reaction import Reaction | ||
|
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.
Stylistically I think the "normal" case should come first in the if/elif/else.
I also think the failure to import pyrms, diffeqpy, or Julia should raise an exception not log a warning - not being able to simulate reactors should stop execution outright IMO. I am aware that other packages currently assume that this import fails and ignore it, but we are burying a significant runtime issue here. Other codes should do something like
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.
Raising an exception to be handled by the calling code would mean the rest of this file would not get imported, which is what this "try/pass" thing is trying to achieve. I think the use case is code that needs to import this module, but doesn't actually execute anything important in it.
Not very satisfactory, I agree, (in more ways than one) but fixing is beyond scope of this PR.