-
Notifications
You must be signed in to change notification settings - Fork 17
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
Pip install . & packaging checks #157
Conversation
…ncies & documentation are included
@timcera please take a look -- I think we should add tooling, but incrementally and as their own separate PRs where the group can discuss. |
@PaulDudaRESPEC combining this PR with #156 should make it so the develop branch has a passing minimal test suite that runs @rburghol can you try running this and see if you can confirm that your command line tests run as expected? |
@PaulDudaRESPEC @timcera I think we should discuss a packaging option that places our three packages behind a single We should also consider what the future is for the HSP2_Driver.py file. This file is only available to users who install from source, and it depends on PyQT5, which we should try to avoid adding to our dependencies. If we have lots of users who depend on this let's move it to another repo and distribute it separately? or document it in an 'example' type setting? It's only ~50 lines of code, but it's a lot of complexity to maintain and distribute. If we do want to keep it, let's re-write it so it's ready for distribution (docs, place the code in a function that is called if name == "main" etc). |
I think I addressed some of these items in #159 |
I agree, thumbs up, sign me on! The three packages are very unusual/unheard of and the only reason I kept them is because I didn't want to (right now) adjust all the imports throughout all of HSP2. But I think a good setup moving forward is the much more common approach to have a single "hsp2" package, organized in source code as "src/hsp2/HSP2", "src/hsp2/HSP2tools", ... etc. I hope I am wrong, but I don't see how to avoid this being an API change and the imports for all scripts and notebooks would have to be re-written.
I would never use this, so I left the care and feeding to others, however I thought the the same questions about it that you have. |
Wow, a lot happened overnight! The HSP2_Driver script is my creation. I thought it was kind of cute to provide an end user a windows dialog to navigate to the UCI/H5 file, but I can see the downside from a packaging perspective... and I think it even predates the CLI implementation. I don't mind removing it from the repo, but you can't make me delete my local copy! |
@timcera and @austinorr, I also share your concern with the way we have 3 separate packages (and use capital letters in the package names). These are all hold-over patterns from the first round of development by Bob Heaphy. I'm also very much a fan of the source layout approach to package structure, which @timcera suggested with ""src/hsp2/hsp2", "src/hsp2/hsp2tools", ... etc." This link to The source layout section of the Python Packages book provides convincing reasons why we should make these changes. Although this would change the way imports are done, I think it's a good idea to make these structural changes sooner than later. The change to imports would be minimal and at the top of the file, i.e. from hsp2 import hsp2, hsp2io, hsp2tools Then we would do a simple search/replace to change from caps to lowercase. If necessary, we can use the |
I would never, haha! It's definitely very cool and fun and worth sharing/preserving. Moving this script to a 'examples' where it can be separated from the main library and improved/revised separately from the library is a nice alternative to removing it from the project completely. |
@aufdenkampe total agreement here. @PaulDudaRESPEC this is a good time to change this, or should we try to get some test coverage before we initiate a refactor that touches imports in every file? I'd be 100% behind this if there was a way to confirm that things work the same after the change as they did before the change. |
@aufdenkampe and @austinorr , I don't have any reservations about this group making these changes sooner than later. No matter what I'll be doing some significant testing before we merge develop into main. |
@PaulDudaRESPEC I’m closing since #156 and #159 cover the fixes this PR suggested. This PR has good discussion on tooling and refactoring into one hsp2 directory that we should revisit, but in another PR. |
Thanks @austinorr -- I am checking through these changes now to ascertain expected behavior. I just suffered the pain of going to 3.10 on one of my instances, but this will incentivize me to get my self in gear! |
Remember talking about this, but I didn't implement this change and wondered what everyone's opinion is. I want to do this but am making sure that a src/hsp2/... refactor is what is agreed on. Before or after the PyPI release? I suggest before, but wait until the special action PR is merged into develop, but this would be the last change before release and packaging. Or wait...? |
@timcera I agree, this should probably happen before a pypi release. Needs approval from @PaulDudaRESPEC and should happen after merging specl-act is merged into develop. I also wonder if you're able to look into the weird numba jit-cache issue where it caches into the install dir and is then un-uninstallable. Maybe there's an accepted work-around we should quickly try to implement before release. |
For the numba issue, I put links in #164 Looks like the only way is to set NUMBA_CACHE_DIR environment variable. Suggest using appdirs.user_cache_dir, which is numba's second choice anyway if numba can't write to the pycache directory. Requires adding appdirs as a dependency. |
@austinorr and @timcera , I hear you... I'll pause after the specact merge next week to circle back to this one. |
@austinorr and @timcera , I'm pausing here as I said, hoping one of you can take on this one last change! |
I can work on it this evening. I think I can finish and have something tomorrow for testing and final polishing. |
ruff
since it does all of isort, black, flake8, and every other imaginable python lint tool all in one -- it's the current standard (scipy, pandas, jupyter, pytest, pip, mypy... on and on.)I attempted to install with current 'develop' branch, several issues:
To fix this i changed
to
and got a new error:
to fix this per the old
setup.py
method, i added _version to py-modules, which makes it importable, but still throws when we try to use the hsp2 command.bundling the top-level VERSION file so that it's importable by our tools is tricky -- notice that now our
_version.py
module lives in the top-level of our site-packages folder:this is not how we are supposed to version our package -- imagine if everyone did this!
Instead we need to find a way to bundle our package so that the version number is both available to modules that need to import/export it, and secondarily make it available for easy modification with future version-bump tooling (I believe this is why it was placed in a VERSION file to begin with).
I believe we should simplify our versioning approach for now and adapt it for tooling once we have consensus for who/how version bumps are made.
This PR has a working distribution that is also a good citizen in our site-packages. However, now our site packages looks like:
In the future we should consider distributing a single package called
hsp2
with each of these modules included as part of that single package.