silx-kit / silx

silx toolkit
http://www.silx.org/doc/silx/latest/
MIT License
136 stars 74 forks source link

hdf5plugin is optional but not in the setup.py #3601

Closed picca closed 2 years ago

picca commented 2 years ago

Hello, hdf5plugin is an optional dependency and this is right.

for info on Debian/Ubuntu/etc.. we are working to install the hdf5 plugin at the system level.

https://packages.debian.org/unstable/hdf5-plugin-lzf
https://tracker.debian.org/news/1301128/accepted-bitshuffle-035-4-source-into-unstable/

This way it now possible to read out of the box the dectris images without special python modules.

Now the setup.py does not considere hdf5plugin as an optional dependency. Due to this the generated entry points try to find hdf5plugin and fail to start silx.

I can maintain a patch on top of silx, but for consistency it seems to me that it should be clear that hdf5plugin is optional, and it should be installed with something like pip install[hdf5plugin], or something else, I am not a user of pip.

so the setup.py must be consistent with the status of hdf5plugin.

Cheers

Fred

t20100 commented 2 years ago

Hi,

we are working to install the hdf5 plugin at the system level.

Great! Those are unfortunately not really meant to be installed otherwise...

By default, hdf5plugin is already not a strong dependency of silx, it is installed through pip install silx[full] and it is not installed with the default pip install silx.

But I guess you are setting the SILX_FULL_INSTALL_REQUIRES env var. which appends [full] requirements to the default ones:

https://github.com/silx-kit/silx/blob/a65330de425a98e8e53c58e7e8b77fc14d4b4650/setup.py#L869-L873

In case the SILX_FULL_INSTALL_REQUIRES env. var. is set, we can exclude hdf5plugin from the requirements. Would that solve your issue?

picca commented 2 years ago

Hello Thomas

Great! Those are unfortunately not really meant to be installed otherwise...

Yes this is the jungle...

By default, hdf5plugin is already not a strong dependency of silx, it is installed through pip install silx[full] and it is not installed with the default pip install silx.

But I guess you are setting the SILX_FULL_INSTALL_REQUIRES env var. which appends [full] requirements to the default ones:

https://github.com/silx-kit/silx/blob/a65330de425a98e8e53c58e7e8b77fc14d4b4650/setup.py#L869-L873

In case the SILX_FULL_INSTALL_REQUIRES env. var. is set, we can exclude hdf5plugin from the requirements. Would that solve your issue?

Yes, I did not remember this :))

here the rules file for Debian

https://tracker.debian.org/media/packages/s/silx/rules-1.0.0dfsg-1

you are right I use this SILX_FULL_INSTALL_REQUIRES=1

the initial purpose of this variable was to add all the dependencies in the install_requires. this way the dh_python3 Debian helper was able to autogenerate the runtime dependencies.

Now I need something else to express the idee, please do not add hdf5plugin to this list.

So maybe another env variable to express this new idea. I do not know if the future build system will allow this sort of things.

or

by default we move all the full_requires in the install_requires except the hdf5plugin one. now the full option just add the hdf5plugin to the list.

I do not know if there is an intereset in the minimalist pip install silx ?

Fred

t20100 commented 2 years ago

I do not know if there is an intereset in the minimalist pip install silx ?

The initial idea was to keep the GUI dependencies optional ("a la" silx-base in conda), since there is a broad range of features in here. This allows e.g., to use the OpenCL-based modules without installing PyQt.

Now I need something else to express the idee, please do not add hdf5plugin to this list.

I propose to change SILX_FULL_INSTALL_REQUIRES to fit your needs, see PR #3602. This env. var. is only used for (Debian) packaging, we could almost rename it SILX_DEBIAN_PACKAGING !

picca commented 2 years ago

I propose to change SILX_FULL_INSTALL_REQUIRES to fit your needs, see PR #3602. This env. var. is only used for (Debian) packaging, we could almost rename it SILX_DEBIAN_PACKAGING !

I think that it is wrong to have Debian in the name of the variable. This is something also usefull for other distribution.

what about a

SILX_INSTALL_REQUIRES_STRIP=hdf5plugin

then the code strip the packages provided by the user.

I agree this is only for hdf5plugin, but I like the idea of something generic like

https://manpages.debian.org/testing/dpkg-dev/dpkg-buildflags.1.en.html

t20100 commented 2 years ago

I think that it is wrong to have Debian in the name of the variable.

I completely agree, that was a joke.

what about a SILX_INSTALL_REQUIRES_STRIP=hdf5plugin

Fine for me.

picca commented 2 years ago

I think that it is wrong to have Debian in the name of the variable.

I completely agree, that was a joke.

Sorry , I do too many things in once...

what about a SILX_INSTALL_REQUIRES_STRIP=hdf5plugin

Fine for me.

Great.

See you soon ;)