Nosetests on Mac fail

[ChrisBNEU]

Bug Description

After PR #2364, on Mac os exclusively, running make test produces a segmentation fault:

mkdir -p testing/coverage
rm -rf testing/coverage/*
nosetests --nocapture --nologcapture --all-modules -A 'not functional' --verbose --with-coverage --cover-inclusive --cover-erase --cover-html --cover-html-dir=testing/coverage --exe rmgpy arkane
ERROR: make: *** [test] Segmentation fault: 11

How To Reproduce

install RMG as specified in our install guide following mac os instructions, then run make test

Expected Behavior

Expect it to run the unit tests

Installation Information

Describe your installation method and system information.

• mac os catalina v10.15.7
  • Apple clang version 12.0.0 (clang-1200.0.32.2)
• RMG version
  • RMG-Py commit: d46a43 (Mar 2)
  • RMG-database commit: f058b7 (Mar 2)

Additional Context

When I comment out the code below from "reactors.py" I am able to run the unit tests:

# 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

[kfir4444]

hey @JacksonBurns, @ChrisBNEU
Maybe the root cause of that issue is nosetests? Nosetests is deprecated, and has been so for a long time (can't find specific deprecation time, but we moved from it in ARC around a year and a half ago). I suspect that more and more issues will rise while using nose, and suggest that you use unittest instead. Here is a merged PR on ARC.

[ChrisBNEU]

@kfir4444 I tested running with/without a debugger and I think it is specifically nosetests that causes the issue, not the debug flag check. figuring out specifically what is causing the segfault would require doing debugging with something meatier like valgrind, and I think for this issue it's not worth it. I think the check for mac in the debug statement that I have in #2393 is an ok fix for now until we upgrade.

[ChrisBNEU]

This is not directly related to this issue, but @JacksonBurns and I were discussing, the bare except statements in that section of reactors.py should be replaced with something more robust. just getting a segfault or "rms.getSpeciesRadius not found" is tough for the average user to decypher.

[JacksonBurns]

Agreed that nosetest is the issue. I think switching from nose to pytest might solve this. I have not tried myself, but pyjulia has a plugin that is supposed to make the two work together: https://pyjulia.readthedocs.io/en/latest/pytest.html#pytest-plugin

Here's the issue from my perspective: we are planning on making this switch on Aug. 1. That means that, until then, we either (1) can't run the unit tests on mac (not great, obviously) or (2) merge #2393 and make it so that mac users cannot use debug mode.

I think we should go a different way entirely. Move the deadline for the pytest switch much sooner and actually fix the underlying problem. I am aware that this will cause huge problems merging outstanding PRs, but I am beginning to think that trying to save ourselves the work of fixing those is creating a lot more work maintaining what we already have.

Ideally, we make the import just look like this and have pytest be compatible (which it should):

import logging
import itertools
from pyrms import rms
from diffeqpy import de
from julia import Main

from rmgpy.species import Species

[calvinp0]

Agreed that nosetest is the issue. I think switching from nose to pytest might solve this. I have not tried myself, but pyjulia has a plugin that is supposed to make the two work together: https://pyjulia.readthedocs.io/en/latest/pytest.html#pytest-plugin

Here's the issue from my perspective: we are planning on making this switch on Aug. 1. That means that, until then, we either (1) can't run the unit tests on mac (not great, obviously) or (2) merge #2393 and make it so that mac users cannot use debug mode.

I think we should go a different way entirely. Move the deadline for the pytest switch much sooner and actually fix the underlying problem. I am aware that this will cause huge problems merging outstanding PRs, but I am beginning to think that trying to save ourselves the work of fixing those is creating a lot more work maintaining what we already have.

Ideally, we make the import just look like this and have pytest be compatible (which it should):

import logging
import itertools
from pyrms import rms
from diffeqpy import de
from julia import Main

from rmgpy.species import Species

If pyrms/diffeqpy and julia are imported without an except statement, it will create issues for ARC and T3.

[JacksonBurns]

Do ARC and T3 use components of RMG other than reactors.py and just rely on that import failing and ignoring it? That's what I'm gathering from a quick glance through the environment.yml and main over there.

How about this instead:

try:
    from pyrms import rms
    from diffeqpy import de
    from julia import Main
except ImportError:
    # tell the user to install Julia or check their installation
    NO_JULIA = True

Later on in execution in RMG, we would need to act differently based on the value of NO_JULIA, i.e. throwing an error if they attempt to use a reactor from RMS that is not available from base RMG.

In ARC and T3, this should not change anything.

I would prefer to have the ImportError stop execution (as an Error typically should), but since enforcing this would require ARC and T3 to have a functioning Julia install I understand why it isn't done that way. I still think it's not a great idea to delay what could be an execution-halting issue when running RMG just to get functionality in other programs, but that's just my opinion.

[kspieks]

@JacksonBurns ARC's goal is to automate QM calculations so I don't believe ARC needs RMG's reactors at all. However, if for some reason ARC imports a function from RMG that eventually cascades into importing something from a file that does import Julia, then it could cause a problem

Once upon a time, @alongd and I had added code to T3 so that it could simulate kinetic mechanisms with RMS reactors.
Although we tried to keep it modularization (i.e. users didn't need to install pyrms or deal with pycall if they wanted to use the RMG or Cantera simulators and weren't interested in the RMS reactors), eventually support for RMS was removed since we have often found it to be nontrivial to deal with pycall...

@Laxzal can you elaborate more about what issues would be raised?

[calvinp0]

Hey @kspieks , it is as you say, T3 and ARC import functions from RMG that will eventually cascade into importing reactors.py, even though it won't be used.

I am a bit more familiar with T3 than ARC, however, I cannot pinpoint which imports are the ones that cause the loading of reactors.py. With that said, IIRC there are two T3 tests (test_function.py) that are good examples of RMG reactors.py being loaded.

[JacksonBurns]

@ChrisBNEU can you checkout the add-macos11-ci branch and try running tests in an environment created with that environment.yml? This PR should resolve the issues mentioned here.

Note that you cannot do make test right now (see #2433 which will fix this) and will instead need to run the unit tests with make test-unittests.

[ChrisBNEU]

@JacksonBurns I did try it, and it is able to run the unittests on mac (catalina 10.15.7, intel processor).

[JacksonBurns]

Thanks for verifying! When #2415 is merged this issue will automatically be closed.