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

Move install_odes from pybamm/ to scripts/ #3385

Closed
Saransh-cpp opened this issue Oct 1, 2023 · 18 comments
Closed

Move install_odes from pybamm/ to scripts/ #3385

Saransh-cpp opened this issue Oct 1, 2023 · 18 comments
Assignees
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours hacktoberfest priority: low No existing plans to resolve

Comments

@Saransh-cpp
Copy link
Member

install_odes.py is a script and is not used by pybamm internally - https://github.com/pybamm-team/PyBaMM/blob/develop/pybamm/install_odes.py

@Saransh-cpp Saransh-cpp added difficulty: easy A good issue for someone new. Can be done in a few hours priority: low No existing plans to resolve hacktoberfest labels Oct 1, 2023
@akhilender-bongirwar
Copy link
Contributor

@Saransh-cpp, I would like to work on this issue. Please assign me : ).

@agriyakhetarpal
Copy link
Member

We will also need to alter the entry point for pybamm_install_odes in this case, because the scripts/ folder is not a module folder

@Saransh-cpp
Copy link
Member Author

I see.

Go ahead, @akhilender-bongirwar! The entry point should be edited here -

PyBaMM/setup.py

Line 293 in 7eb5879

"pybamm_install_jax = pybamm.util:install_jax",

@akhilender-bongirwar
Copy link
Contributor

akhilender-bongirwar commented Oct 1, 2023

I see.

Go ahead, @akhilender-bongirwar! The entry point should be edited here -

PyBaMM/setup.py

Line 293 in 7eb5879

"pybamm_install_jax = pybamm.util:install_jax",

Hey @Saransh-cpp, I think I should change the entry point of this line 292 - "pybamm_install_odes = pybamm.install_odes:main",

@agriyakhetarpal , should I remove this line from entry point as the scripts/ is not a module folder.
Please correct me if I am wrong.

@agriyakhetarpal
Copy link
Member

Yes, that is correct, it should be the line above the one @Saransh-cpp quoted. However, we shouldn't remove the entry point altogether at this moment. Could you try adding an entry point to install_odes inside the scripts/ folder (and investigate if that is possible)?

@akhilender-bongirwar
Copy link
Contributor

Yeah @agriyakhetarpal, I shall investigate.

@akhilender-bongirwar
Copy link
Contributor

@agriyakhetarpal, according to what I have investigated I am sure that entry point to install_odes can be added to scripts/docker file to RUN if [ "$ALL" = "true" ]; then \ we can add at line 59 as python scripts/install_odes.py && \ as the entry point.

Please let me know if I am right about this :)

@agriyakhetarpal
Copy link
Member

We don't need to edit the Dockerfile in this case. This is related to PyBaMM's packaging infrastructure, the Docker installation picks it up as needed. pybamm_install_odes is an entry point, i.e., an app shipped with and enabled as such with the package, which can be run inside the root directory to install the build-time requirements (SUNDIALS) and an optional solver (scikits.odes)

@akhilender-bongirwar
Copy link
Contributor

@agriyakhetarpal, I understand. So, it seems there's no option to include 'install_odes' in the 'scripts/' folder then.

@agriyakhetarpal
Copy link
Member

@Saransh-cpp I am happy to close this since entry points don't work outside importable module folders, unless you have some other ideas :) We shouldn't deprecate this command at the moment because it is useful for Linux after all.

@Saransh-cpp
Copy link
Member Author

Oh yes, it would be nice to make this script "uniform" with other scripts (like install_KLU_Sundials.py), that is, remove this as an entry point and encourage users to use python scripts/install_odes.py. If everyone thinks that keeping the script as an entry point is better than please feel free to close this!🙂

@agriyakhetarpal
Copy link
Member

A hacky way around this would be to put the functionality in scripts/install_odes.py, switch to pybamm.root_dir(), and call it from the original pybamm/install_odes.py in a subprocess while capturing stdout and stderr in order to retain the pybamm_install_odes entry point. My preference would be towards keeping it as-is, though.

@akhilender-bongirwar
Copy link
Contributor

@Saransh-cpp, could you please summarize what exactly should I be doing to complete this issue and raise a PR.

@arjxn-py
Copy link
Member

arjxn-py commented Oct 2, 2023

it would be nice to make this script "uniform" with other scripts (like install_KLU_Sundials.py), that is, remove this as an entry point and encourage users to use python scripts/install_odes.py.

This way we can encourage only developers to use python scripts/install_odes.py, removing pybamm.install_odes:main as an entrypoint might no longer leave pybamm_install_odes as user facing.

@akhilender-bongirwar
Copy link
Contributor

@arjxn-py , are you suggesting me to remove the entry point pybamm.install_odes.main ?

@arjxn-py
Copy link
Member

arjxn-py commented Oct 3, 2023

are you suggesting me to remove the entry point pybamm.install_odes.main ?

No, i was suggesting to avoid removing this entrypoint as if we remove it then users might not have any way to access python scripts/install_odes.py

@Saransh-cpp
Copy link
Member Author

Oh yes, that is true, +1 on keeping things as it is. Closing this issue.

@Saransh-cpp Saransh-cpp closed this as not planned Won't fix, can't repro, duplicate, stale Oct 3, 2023
@agriyakhetarpal
Copy link
Member

Feel free to work on the other issue you commented on, @akhilender-bongirwar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours hacktoberfest priority: low No existing plans to resolve
Projects
None yet
Development

No branches or pull requests

4 participants