Skip to content
This repository was archived by the owner on Mar 25, 2025. It is now read-only.

Fix Python discovery issue when calling sympy solver from wheel#1165

Merged
JCGoran merged 19 commits into
masterfrom
jelic/fix_sympy_visitor
Feb 27, 2024
Merged

Fix Python discovery issue when calling sympy solver from wheel#1165
JCGoran merged 19 commits into
masterfrom
jelic/fix_sympy_visitor

Conversation

@JCGoran

@JCGoran JCGoran commented Feb 22, 2024

Copy link
Copy Markdown
Contributor

(Splitting this up from #1147 as it was getting too large).

As an example (stolen from one of the notebooks):

# example.py
import nmodl.dsl as nmodl


def run_conductance_visitor_and_return_breakpoint(mod_string):
    # parse NMDOL file (supplied as a string) into AST
    driver = nmodl.NmodlDriver()
    AST = driver.parse_string(mod_string)
    # run SymtabVisitor to generate Symbol Table
    nmodl.symtab.SymtabVisitor().visit_program(AST)
    # constant folding, inlining & local variable renaming passes
    nmodl.visitor.ConstantFolderVisitor().visit_program(AST)
    nmodl.visitor.InlineVisitor().visit_program(AST)
    nmodl.visitor.LocalVarRenameVisitor().visit_program(AST)
    # run CONDUCTANCE visitor
    nmodl.visitor.SympyConductanceVisitor().visit_program(AST)
    # return new BREAKPOINT block
    return nmodl.to_nmodl(
        nmodl.visitor.AstLookupVisitor().lookup(
            AST, nmodl.ast.AstNodeType.BREAKPOINT_BLOCK
        )[0]
    )


ex1 = """
NEURON {
    USEION na READ ena WRITE ina
    RANGE gna
}
BREAKPOINT {
    ina = gna*(v - ena)
}"""
print(run_conductance_visitor_and_return_breakpoint(ex1))

Running this with a freshly-installed wheel on latest master (built with python setup.py bdist_wheel) gives:

$ python example.py
[NMODL] [critical] :: NMODL_PYLIB environment variable must be set to load embedded python
Traceback (most recent call last):
  File "/private/var/folders/r_/1lcd8hvd6nzgtzcy8wwq9slx186djn/T/tmp.vzUItTIK/example.py", line 32, in <module>
    print(run_conductance_visitor_and_return_breakpoint(ex1))
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/private/var/folders/r_/1lcd8hvd6nzgtzcy8wwq9slx186djn/T/tmp.vzUItTIK/example.py", line 15, in run_conductance_visitor_and_return_breakpoint
    nmodl.visitor.SympyConductanceVisitor().visit_program(AST)
RuntimeError: NMODL_PYLIB not set

Basically, due to the way loading of Python is handled, we should set the various env variables as soon as we load the module, otherwise the user may find mysterious errors.

Other changes:

  • updated requirements and setup.py (for importlib)
  • added a test for env variables (needs a BREAKPOINT statement to activate the sympy solver)
  • added automatic setting of NMODL_PYLIB and NMODLHOME env variables (only if they are not already set!)

Goran Jelic-Cizmek and others added 3 commits February 21, 2024 17:52
When using the sympy visitor, the NMODL Python module by default throws
an error that `NMODL_PYLIB` is not set. The solution is to set all of
these env variables as soon as we load the module.
@bbpbuildbot

This comment has been minimized.

Comment thread nmodl/__init__.py
@JCGoran

JCGoran commented Feb 22, 2024

Copy link
Copy Markdown
Contributor Author

Need BlueBrain/spack#2320 to get merged, then the gitlab pipeline should be ✅
EDIT: Spack PR got merged, all green now :)

@bbpbuildbot

This comment has been minimized.

@codecov

codecov Bot commented Feb 22, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (bc913ef) to head (8e7c5ca).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1165   +/-   ##
=======================================
  Coverage   88.40%   88.40%           
=======================================
  Files         175      175           
  Lines       12934    12934           
=======================================
  Hits        11434    11434           
  Misses       1500     1500           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbpbuildbot

This comment has been minimized.

@JCGoran JCGoran marked this pull request as ready for review February 23, 2024 09:33
@bbpbuildbot

This comment has been minimized.

@JCGoran JCGoran requested a review from 1uc February 23, 2024 10:40
Comment thread nmodl/__init__.py Outdated
Comment thread requirements.txt
Comment thread test/unit/pybind/test_env_variables.py Outdated

@1uc 1uc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks very sensible to me. My comment are all optional, please feel free to ignore them, if you think they don't make sense.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

Copy link
Copy Markdown
Collaborator

Logfiles from GitLab pipeline #195941 (:white_check_mark:) have been uploaded here!

Status and direct links:

@codecov-commenter

codecov-commenter commented Feb 26, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.17%. Comparing base (2cff7e0) to head (f8d41dd).

❗ Current head f8d41dd differs from pull request most recent head 62b93f0. Consider uploading reports for the commit 62b93f0 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1165   +/-   ##
=======================================
  Coverage   87.17%   87.17%           
=======================================
  Files         175      175           
  Lines       12884    12884           
=======================================
  Hits        11232    11232           
  Misses       1652     1652           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JCGoran

JCGoran commented Feb 27, 2024

Copy link
Copy Markdown
Contributor Author

Minor annoyance: if we merge this, we need to add py-importlib-resources to the NMODL Spack recipe (only if we test on Python 3.8, if not, you can ignore this).

@1uc

1uc commented Feb 27, 2024

Copy link
Copy Markdown
Collaborator

That doesn't seem wrong. The dependencies changed, hence the Spack recipe needs to change.

Comment thread docs/contents/currents.rst
@JCGoran JCGoran merged commit 1baa906 into master Feb 27, 2024
@JCGoran JCGoran deleted the jelic/fix_sympy_visitor branch February 27, 2024 21:18
ohm314 pushed a commit that referenced this pull request May 21, 2024
* Fix for Python sympy solver

* Add corresponding test

* Update requirements

---------

Co-authored-by: Luc Grosheintz <luc.grosheintz@gmail.com>
JCGoran added a commit to neuronsimulator/nrn that referenced this pull request Mar 12, 2025
* Fix for Python sympy solver

* Add corresponding test

* Update requirements

---------

Co-authored-by: Luc Grosheintz <luc.grosheintz@gmail.com>

NMODL Repo SHA: BlueBrain/nmodl@1baa906
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants