search

RNO-G / mattak

RNO-G dataformats
1 stars 1 forks source link

Make mattak pip-installable #31

Closed sjoerd-bouma closed 4 months ago

sjoerd-bouma commented 4 months ago

Both mattak backends are now pip-installable! I don't know how helpful this is, considering the uproot backend was already pip-installable, and installing root will probably remain the bigger hurdle for most people, but for those with a working ROOT installation installing pip is now as simple as

pip install git+https://github.com/RNO-G/mattak.git

(well, for the moment one has to add @make-pip-installable to point pip to this branch). If @cozzyd has a pypi account it would be straightforward to enable pip install mattak as well, if this is something we want.

The backend that is responsible for the pip installation uses CMakeLists.txt; the only relevant change I made there is that the installation will now automatically fall back on a ROOTless installation if ROOT cannot be found (Cmake will just emit a warning). We could potentially make this dependent on whether the installation is with pip or with make && make install.

sjoerd-bouma commented 4 months ago

As a bonus - I added a github action that tests the installation and runs the test_dataset.py example :upside_down_face: for both uproot and pyroot backends. It currently runs for every push, but we can make this more restrictive if we don't want to use more github minutes.

cozzyd commented 4 months ago

This looks pretty good. scikit-build is probably more effective than my attempt to using cmake directly in setup.py that is probably hard to make work on multiple platforms.

A few notes:

sjoerd-bouma commented 4 months ago

In principle, in the case when PyROOT is available, it should probably build the ROOT-less version for uproot anyway? That would at the very least make testing a lot easier. So we probably need to instruct scikit-build to build it twice, allowing one to fail (or maybe it's possible to guard it by the equivalent of trying to see if import ROOT succeeds.... I have no idea if that's easy or not with pyproject.toml). Though actually, I guess it's probably just easier to put all such logic in CMakelists.txt now that setup.py is deprecated...

Yes, the most straightforward would be to do this in CMakelists.txt. We can just remove the if(ROOTLESS) condition or replace it with if(ROOTLESS OR PIP_INSTALLATION) depending on whether we want to have this behaviour always or only for the pip-installed version.

cozzyd commented 4 months ago

Yes that makes sense, I can try to implement those changes unless you do it first!

I have also requested an rno-g PyPI organization FYI

cozzyd commented 4 months ago

alright, mattak (from this branch, obviously) is now on PyPI... minimal testing indicates it might work?

cozzyd commented 4 months ago

ok I just yanked it (twice... with two slightly different versions since it makes you change the version).

There are at least two problems:

cozzyd commented 4 months ago

Even if we can get pip to always build from source and ignore the precompiled wheel, since ROOT doesn't maintain any ABI compatibility even across patch versions, if ROOT gets updated, then libmattak.so must be rebuilt, which might be tricky to orchestrate. I guess on clusters this is not that big of an issue since ROOT is likely updated irregularly, if at all, but people getting ROOT from EPEL / Fedora repositories (like I do...) get regular ROOT updates, making pip a poor choice for that. I guess that's just something we can document though?

cozzyd commented 4 months ago

ok, maybe this works now? I can just not upload the wheel with twine (I guess it wouldn't have worked for most people anyway unless they were using Python 3.9 which I have on my workstation) to force a source build, and libmattak_noroot.so wasn't being installed because I didn't have an install target.

sjoerd-bouma commented 4 months ago

Yes, I wondered if uploading the wheel might not be a great idea. Re ROOT updating - I didn't think of this, but I guess most people that would benefit from mattak being pip-installable wouldn't regularly try to update ROOT. Still would be good to document this as you suggest though.

Also, if you think it would be useful it would be straightforward to also automate the uploading to pypi - if we do this the same way as NuRadioMC then it could run on every push to main, for example.

cozzyd commented 4 months ago

Yeah, I guess we could separate the two backends and only have a wheel for the uproot backend, but then we still need wheels for every OS/arch/cpython version to be super useful, and I think we can assume people have a c++ compiler available?

As for automatic package creation, it's not clear to me how to deal with versioning. PyPI doesn't seem to like it when you replace an old version, so we'd have to bump the version number with each push to main, which to me feels too frequent at this point. Maybe we can create a "stable" branch that can have that property...

fschlueter commented 4 months ago

As for automatic package creation, it's not clear to me how to deal with versioning. PyPI doesn't seem to like it when you replace an old version, so we'd have to bump the version number with each push to main, which to me feels too frequent at this point. Maybe we can create a "stable" branch that can have that property...

I agree, or we adopt a similar workflow as in NuRadio and introduce a develop branch.

cozzyd commented 4 months ago

sure maybe we rename main to 'develop', and create a 'release' branch that gets pushed to PyPI automatically?

cozzyd commented 4 months ago

I will go ahead and merge this for now, though we should keep a discussion about automatic deployment / branch renaming (perhaps in a new pull request or an issue?).