PySlurm / pyslurm

Python Interface to Slurm
https://pyslurm.github.io
GNU General Public License v2.0
491 stars 119 forks source link

Support Build Requirements via PEP 517 / pyproject.toml #190

Closed salotz-sitx closed 2 years ago

salotz-sitx commented 3 years ago

The goal is to have pip install pyslurm just work without any preparation. This allows for pyslurm to be put into environment specification files like requirements.txt and have environments built in one step, which is useful for CI/CD etc.

Instead of having users install Cython before installing pyslurm, you can specify what the build requirements are (according to PEP 517) in a file pyproject.toml. E.g.:

[build-system]
requires = ['setuptools', 'wheel', 'cython']

I tried doing this, but there is a lot of extra logic for checking for Cython versions in the setup.py that stops the build before it can trigger this on a pip install .

Another blocker here is the reliance on arguments for --slurm, --slurm-lib, and --slurm-inc. I haven't found a way to have pip forward on arguments like this to the python setup.py build command. This should probably be dealt with using environment variables instead of command line arguments so I can configure my build/installation environment around tools like pip that will be installing pyslurm.

giovtorres commented 3 years ago

What if we were to ship the Cython generated C code in the sdist package, would that potentially make it easier to install?

With respect to the lib and inc arguments, if slum libraries and headers are installed in non-standard locations, we can suggest to the user to setup their ld.so.conf.d file to include slurm paths, or we could ask users to export the LD_LIBRARY_PATH and CPATH environment variables before instally pyslurm via pip.

Is getting the project compatible with PEP 517 something you'd be willing to look into and contribute?

salotz-sitx commented 3 years ago

I tried to edit the setup.py but it wasn't working so I didn't PR. If you want to see where I was going I can do so.

I think generating the C code is a fine approach as well, I guess its up to you. I'm not sure the details of whether or not the code generation can happen at packaging time or not. If that works it should be the most robust way.

tazend commented 2 years ago

I also think that shipping the generated c code could be a big improvement in this case. Also from the documentation here, this is pretty much recommended:

It is strongly recommended that you distribute the generated .c files as well as your Cython sources, 
so that users can install your module without needing to have Cython available.

In addition to the link I sent above, it's also recommended to make usage of cython during install optional (and therefore disabled by default).

giovtorres commented 2 years ago

Thanks for the link, @tazend. I also discussed this with @gingergeeks a couple days ago and I think we are all in agreement here. We should definitely work towards this. I think it will help us get to a point where a user can just run pip install pyslurm and it would install with no dependency on Cython.

I'm also slowly becoming a fan of pyproject.toml. I'd love to see support for this in PySlurm!

Thanks for raising this issue. If anyone wants to take it, go for it!

salotz-sitx commented 2 years ago

Great news, this would help us streamline our deployments out a lot. Once there is a solution for master branch would also like to see it backported to older releases since for us at least we use an older version of SLURM.

I went ahead and made a WIP draft PR #203 . Might be useful, probably isn't though.

mtwest2718 commented 2 years ago

Does pip & python natively support .toml files yet or do you need to install https://pypi.org/project/toml/ first?

salotz-sitx commented 2 years ago

@mtwest2718 AFAIK there is nothing in the standard library. I believe it would need to be the responsibility of the build tool (for which pyproject.toml facilitates using different things other than setuptools) or project introspection tool. I think most of them pull in a toml library as a dependency. I've never needed to manually install the toml library for using pyproject.toml.

mtwest2718 commented 2 years ago

So I should in principle be able to create a venv and just pip install the toml file without first grabbing any other packages?

salotz-sitx commented 2 years ago

I think you are misunderstanding the purpose of the pyproject.toml. Its essentially a standardized version of a package specification. It allows for projects to use different tools for building projects. E.g. you can use poetry instead of setuptools. I think setuptools is the default if you don't specify a different tool.

The purpose of the pyproject.toml here though is really besides that point. You just use it to declare build-time dependencies. A PEP 517 builder (i.e. pip or build will read the pyproject.toml (from the source or sdist as it may be) see what the build dependencies are, create a temporary isolated virtualenv, build your project in there. In the case of pip it will usually install it as well.

So there isn't much difference between this and the build_deps option in setuptools (assuming that ever worked idk I never used it), except this one actually is supported and standardized.

alanhoyle commented 2 years ago

What would it take to make pip install the current version? When one does pip install pyslurm right now, they end up with a PySlurm version around 18.8.1.1 and a version of cython that is old enough that installing the current PySlurm from source doesn't work. After a recent cluster update, I had to do the following to make PySlurm work again:

source /path/to/my-venv/bin/activate # activate my python venv
pip uninstall cython pyslurm  # uninstall broken pyslurm and older cython
pip install cython # installs the latest version
git clone https://github.com/PySlurm/pyslurm.git # get the pyslurm source
cd pyslurm 
python setup.py build
python setup.py install

Before updating PySlurm, when I tried to run the older version and got the following error:

Traceback (most recent call last):
  File "/path/to/myscript.py", line 11, in <module>
    import pyslurm
  File "/path/to/my-venv/lib/python3.7/site-packages/pyslurm/__init__.py", line 16, in <module>
    from .pyslurm import *
ImportError: libslurmdb.so.33: cannot open shared object file: No such file or directory
tazend commented 2 years ago

Hi @alanhoyle ,

to get the latest pyslurm version(s) released to PyPi, we'd need to ask @giovtorres on this perhaps (I don't have access to the Account there).

Now, as mentioned in this issue already there are a few things that would probably be benificial to have in advance to that:

  1. Finding libslurm.so when Slurm is installed in non-standard locations. For example, direct the user to set specific environment variables that will be picked up during installation process. Of course, the most important thing is that this is clearly documented.
  2. By default, leaving out Cython as a direct dependency for installation, since we could generate the C-Code on our side, and ship it with the releases. That could simplify installation a lot and makes sure that cython code is consistent for all users. Only thing the user needs would be a C-compiler, e.g gcc. Of course, there should still be a way to let the user choose (e.g. via env-vars) if they want to generate the code with Cython themselves.

Of course, working towards a more modern build system via PEP 517 for example will definitely benefit the project. I might have a look at those soon and see whats necessary and possible.

In regards to your ImportError: You cannot use the 18.8 pyslurm release (latest on pypi) with newer versions of slurm installed (>= 19.05), because libslurmdb.so doesn't exist there anymore and was merged into libslurm.so.

tazend commented 2 years ago

Currently working on this and have already done a bit of rework to implement some of the functionality wantedhere to have a smoother install-process (adding pyproject.toml, accepting lib-dir and include-dir via env-vars, ...).

Whats left is just to make it so the generated C-code is shipped with the source-dist and Cython-codegen is disabled by defaullt when installing (though we can give an Option to do the codegen yourselves if one really wants to), so the only real thing a user needs is a c- compiler during installation.

giovtorres commented 2 years ago

I missed an earlier comment about the latest available version of pyslurm available via pip. This needs to be part of the build system. I was trying to accomplish this with TravisCI but never fully implemented it. Ideally, each merge to the main branch should trigger a version bump and a push to pypi. Perhaps a GitHub Action would work, didn't have those back then.

salotz-sitx commented 2 years ago

Thanks for implementing this. Will make it much easier to add PySlurm as a dependency.