-
Notifications
You must be signed in to change notification settings - Fork 0
Packaging as namespace packages #13
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
Conversation
@cssherman I have finished the work for this PR
|
I'll start taking a look! I don't see any issues with the name change |
docs/conf.py
Outdated
@@ -18,7 +18,7 @@ | |||
|
|||
# Add python modules to be documented | |||
python_root = '..' | |||
python_modules = ( 'geosx_mesh_tools_package', 'geosx_xml_tools_package', 'geosx_mesh_doctor', 'geos_ats_package', | |||
python_modules = ( 'geosx_mesh_tools_package', 'geosx_xml_tools_package', 'geosx_mesh_doctor', 'geos-ats_package', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package names, paths that are used in the python autodocs need to be updated. We can easily add a CI test to make sure docs build correctly before deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a CI workflow to check the documentation.
@@ -1,5 +1,5 @@ | |||
import numpy as np | |||
from hdf5_wrapper import hdf5_wrapper as h5w | |||
from geos.hdf5.wrapper import hdf5_wrapper as h5w |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the name changes, shouldn't this be:
from geos.hdf5wrapper import wrapper as h5w
Since we are currently only testing package installation in the CI, we need to do a bunch of manual tests to make sure that the changes work. Also, @untereiner - could you open up a testing branch in the main GEOS repository, following these instructions: |
Before we do this, we need to clarify the license information in these packages / the top-level repo. We also need to check with @rrsettgast and others to get approval to do so. |
@untereiner - I'm also happy to setup the pip repository. I'm already managing one for another project, so I'm comfortable with the tools/workflows. |
The CI of the GEOS repo fails for this branch because I changed the names of the modules, Then, the integrated tests don't find the geos-ats module. |
What are the failures ? pyproject.toml is standard |
Yes, but this is a pretty new standard. Using a version of pip that was only 2 versions old, this lead to silent errors that were very hard to debug. I don't think there is a way to enforce this, so we need to add some big warning boxes to the docs. |
When you say two versions old, you start with python 3.12 ? |
@untereiner - do we have regular users of mesh doctor that we can call on to test these changes? We don't have any integrated tests involving that package in the main repo (yet). |
@untereiner - Looking at these changes again, there were a handful of module name changes that added more depth that I like in the namespace. There were also a number of instances where the changes weren't reflected properly in test scripts, documentation, etc. I've done some additional work to address this in my recent commit. |
I've also fixed some doc build issues, and consolidated some of the older mesh conversion tools and the doctor into a single 'geos-mesh' package |
I meant two versions old of pip (obviously not an ideal situation, but this was on a CI machine that may not have been updated in a while). While And with respect to documentation, I agree that we should note this behavior. For most users though, we just need to include a For python version support, I think that we need to support all non-deprecated versions of python. Some end-users may not have full control of their environments and we should do our best to make their lives easier. |
@CusiniM - do you have any thoughts about the re-organization of these packages? The big change here is how they would be imported. For example, |
@jhuang2601 and I can test this. However the installation was already not working. |
I don't think this is an issue. However, is there a specific reason for creating all those nested folders? |
I haven't packaged something like this before, but from what I understand, this allows us to independently package components of a single larger module. Maybe this has benefits for simplifying testing? |
They can be packaged, tested, distributed separately but they can work together easily because they share the same root. It’s a kind of separation of concerns. |
@cssherman For python versions it means min 3.8 until 2024/10
What can I do now to help move forward ? |
I think that this is ready to move forward. I've added the associated PR in the main GEOS repo to the dashboard, with a note on the procedure to handle the merge. We also need to have a discussion with the larger GEOS community to give them an explanation of the changes, so they aren't caught off-guard. And WRT python versions, I agree that in a perfect world, we should only need to target reasonably up-to-date versions of python. At least in short-term, I think we need to keep with the current strategy. |
@cssherman great! |
No description provided.