search

mikekatz04 / BBHx

GNU General Public License v3.0
17 stars 15 forks source link

Questions on Python requirements and install instructions #16

Open titodalcanton opened 1 month ago

titodalcanton commented 1 month ago

PyCBC runs a variety of CI tests on various Python versions, currently starting from 3.9, and some of these tests require BBHx. BBHx's recent requirement of Python 3.12 means that most of those tests now fail. I inspected BBHx's master branch after the introduction of the 3.12 requirement, and I was wondering if Python 3.12 is actually necessary. In fact, with a minimal modification, I managed to install BBHx in a Python 3.9 virtualenv, though I have not tested it [1].

Part of that modification included the realization that I had to run scripts/prebuild.py prior to running pip install .. This seems to be a recent change, not documented (yet?). I am not sure what the idea behind that extra step is, so I simply added an invocation of scripts/prebuild.py to setup.py.

I also dropped a coupe unnecessary imports, removed a stray Jupyter checkpoint that had been accidentally committed into the repository, and added a basic .gitignore file to prevent unwanted files from accidentally ending up in the repository.

The changes are in https://github.com/titodalcanton/BBHx/tree/py39-and-cleanup. If this all sounds ok, I can open a pull request.

[1] I did try running bbhx/tests/test_bbhx.py, but got ModuleNotFoundError: No module named 'lisatools.sensitivity', which I think is unrelated to the change I made.

mikekatz04 commented 3 weeks ago

Hi @titodalcanton , thank you for pointing these out. These were the result of some hasty changes, whoops! The scrips/prebuild setup has to do with installing GPU/CPU stuff while also allowing for python -m build to do its work. That is at least what I found to work. Not sure if adding that to setup.py messes up the building process. I have now fixed a variety of things related to this including most importantly the removal of the Python 3.12 requirement. Sorry about that!!!! It does require lisaanalysistools now as that houses the detector orbital information. Let me know what you think about how this meshes with your pull request and we can go forward from there.

titodalcanton commented 3 weeks ago

Thanks @mikekatz04.

Regarding the Python version requirement, I still see python_requires=">=3.12" in the current setup.py. That is the critical bit that PyCBC needs to change.

Regarding scripts/prebuild.py, all it does is copy some files around and create a couple other files, so it should be idempotent as far as I can tell.

Anyway, it sounds like I should update my branch with your latest changes, and open a PR.

mikekatz04 commented 3 weeks ago

Hi @titodalcanton , sorry! I made the code updates and forgot to adjust that line in the setup.py file. It is now updated to >=3.6 I think for now so it should work (with version 1.1.1)