search

lanl / LaGriT

Los Alamos Grid Toolbox (LaGriT) is a library of user callable tools that provide mesh generation, mesh optimization and dynamic mesh maintenance in two and three dimensions.
https://lanl.github.io/LaGriT/
Other
116 stars 48 forks source link

Why is pexpect included in the source? #195

Closed banesullivan closed 4 years ago

banesullivan commented 4 years ago

Why is pexpect's source code included in the PyLaGriT package?

From a quick glance, it isn't modified (is it?). Why not just list it as a dependency in the setup.py?

If a specific version is needed, you can set that in the setup.py.

dharp commented 4 years ago

Hi Bane,

Yes, all good advice. In the beginning, I just packaged it with PyLaGriT to avoid having to deal with dependencies. Since the pexpect that we include does everything we need, I guess I’m still leaning towards just leaving it as is and avoiding dependency issues. I will keep the suggestion in mind though as we move forward.

Thanks, Dylan

From: Bane Sullivan notifications@github.com Reply-To: lanl/LaGriT reply@reply.github.com Date: Wednesday, April 15, 2020 at 7:53 AM To: lanl/LaGriT LaGriT@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: [EXTERNAL] [lanl/LaGriT] Why is pexpect included in the source? (#195)

Why is pexpect's source code included in the PyLaGriT package?

From a quick glance, it isn't modified (is it?). Why not just list it as a dependency in the setup.py?

If a specific version is needed, you can set that in the setup.py.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/lanl/LaGriT/issues/195, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AASX6BHMF2RBCABNV3GEOKLRMW34BANCNFSM4MIS7AUQ.

banesullivan commented 4 years ago

I went ahead with a PR to take it out and use pip to manage the dependency (see #196) - installing the package this way is painless.

Having pexpect included in the repo has no advantage: it just adds more files to the repo and confusion for new developers looking to modify the source. Also, this brings inconsistency as numpy is a requirement too. If including pexpect, why not include numpy?

And soon I will add some routines to have 3D visualization with PyVista for #161 and having PyVista as an optional dependency in the setup.py. Having all of this managed by pip will make the installation process much simpler for end-users.

I'm willing to put in the time to properly set up the Python package to follow best practices. Also, I would like to set up the build process so the lagrit binary and Python interface can easily be shipped to PyPI for easy installation by Python users.

dharp commented 4 years ago

Sounds good. Thanks for all the efforts. BTW, who are you?

Dylan

From: Bane Sullivan notifications@github.com Reply-To: lanl/LaGriT reply@reply.github.com Date: Wednesday, April 15, 2020 at 8:48 AM To: lanl/LaGriT LaGriT@noreply.github.com Cc: "Harp, Dylan Robert" dharp@lanl.gov, Comment comment@noreply.github.com Subject: [EXTERNAL] Re: [lanl/LaGriT] Why is pexpect included in the source? (#195)

I went ahead with a PR to take it out and use pip to manage the dependency (see #196https://github.com/lanl/LaGriT/pull/196) - installing the package this way is painless.

Having pexpect included in the repo has no advantage: it just adds more files to the repo and confusion for new developers looking to modify the source. Also, this brings inconsistency as numpy is a requirement too. If including pexpect, why not include numpy?

And soon I will add some routines to have 3D visualization with PyVista for #161https://github.com/lanl/LaGriT/issues/161 and having PyVista as an optional dependency in the setup.py. Having all of this managed by pip will make the installation process much simpler for end-users.

I'm willing to put in the time to properly set up the Python package to follow best practices. Also, I would like to set up the build process so the lagrit binary and Python interface can easily be shipped to PyPI for easy installation.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/lanl/LaGriT/issues/195#issuecomment-614084494, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AASX6BB6SHR73WJT6FUYE3TRMXCKPANCNFSM4MIS7AUQ.

banesullivan commented 4 years ago

I'm a developer for @pyvista, a geophysicist, and at the moment I'd like to see LaGriT grow in the Python ecosystem. I've interacted with @daniellivingston here in the past

dharp commented 4 years ago

Great, good to have you working with us Bane!

Thanks, Dylan

From: Bane Sullivan notifications@github.com Reply-To: lanl/LaGriT reply@reply.github.com Date: Wednesday, April 15, 2020 at 9:00 AM To: lanl/LaGriT LaGriT@noreply.github.com Cc: "Harp, Dylan Robert" dharp@lanl.gov, Comment comment@noreply.github.com Subject: [EXTERNAL] Re: [lanl/LaGriT] Why is pexpect included in the source? (#195)

I'm a developer for @pyvistahttps://github.com/pyvista, a geophysicist, and at the moment I'd like to see LaGriT grow in the Python ecosystem. I've interacted with @daniellivingstonhttps://github.com/daniellivingston here in the past

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/lanl/LaGriT/issues/195#issuecomment-614091854, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AASX6BCDDOEK4ORDPL3N4VTRMXDYFANCNFSM4MIS7AUQ.

daniellivingston commented 4 years ago

Thanks @banesullivan !