Skip to content
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

fix(imports): only import matplotlib where needed and if gui is enabled #788

Conversation

sanjayankur31
Copy link
Contributor

Fixes #786

Same as #787, but cherry-picked to apply against development

@sanjayankur31
Copy link
Contributor Author

A simple test script to check matplotlib imports is here (maybe this should be added as a test in CI?)

"""
Example python script to run on NSGR

File: example.py

Copyright 2023 OSB contributors
"""


from netpyne import specs
from netpyne import sim
from netpyne import __version__ as version

import sys
import pkg_resources

if __name__ == "__main__":
    print("Running example script to list Python packages...")

    with open("output.txt", "w") as f:
        print(f"Python version is: {sys.version}", file=f)
        print("Installed packages on NSG:", file=f)
        dists = [str(d).replace(" ", "==") for d in pkg_resources.working_set]
        for i in dists:
            print(i, file=f)

    for k in sys.modules.keys():
        if "matplotlib" in k:
            print(f"matplotlib still loaded: {k}")

    print(f"Args are: {sys.argv}")
    if sys.argv.count("-nogui") > 0:
        print("nogui found")
    print("Done!")

This is required to prevent matplotlib from being imported on HPCs.
Copy link
Collaborator

@jchen6727 jchen6727 left a comment

Choose a reason for hiding this comment

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

reviewed the netpyne/analysis, netpyne/plotting, netpyne/support
looks good, probably will receive NameError: name 'plt' is not defined now for many of the plotting functions if they are called by accident with -nogui
if @vvbragin

@vvbragin
Copy link
Collaborator

vvbragin commented Nov 7, 2023

@sanjayankur31 @jchen6727 thank you guys, looks good to me as well. For those plotting functions highlighted by James, where plt is needed, I would move the import inside the functions themselves, so the importing won't get triggered unless user explicitly wants to call that func. (Conceptually, I wouldn't connect the plotting functions to -nogui option, because the latter is not about plotting but specifically about GUI as far as I understand). What do you think?

Anyways, I'd merge this one PR, and the rest of modifications I would make in separate one (it's just easier for me then dealing with forked repo's PRs)

@vvbragin vvbragin merged commit a3cf980 into suny-downstate-medical-center:development Nov 7, 2023
2 checks passed
@sanjayankur31
Copy link
Contributor Author

Hello,

Yes, moving the imports into the functions is probably the best thing to do---or if the main netpyne __init__ imports the necessary matplotlib bits, then the other sub-modules could just use it from there, so you don't need to re-import things again and again (like the __gui__ variable). In general, it's probably a good idea to keep the analysis bits separate from the modelling/simulation bits, unless one can only use the analysis bits if they've first created a model and run a simulation..

@sanjayankur31 sanjayankur31 deleted the fix/786-development branch November 7, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants