Remove `pynvml` module and convert to a "meta package" #57

rjzamora · 2024-08-13T17:00:59Z

TODO:

Confirm "correctness" of meta-package approach
Update documentation

rjzamora · 2024-08-13T17:05:12Z

NOTE: When I install this package, I cannot query the version of pynvml (AttributeError: module 'pynvml' has no attribute '__version__'). Not sure if any down-stream users depend on this functionality, but nvidia-ml-py doesn't seem to support it.

@jakirkham - Does this general approach seem reasonable? Also, if we start depending on nvidia-ml-py, do we also need to make changes in the nvidia-ml-py feedstock?

jakirkham · 2024-11-22T02:17:09Z

Think we are using Versioneer here. We would want to follow these steps as if we were configuring versioneer from scratch

It is possible we will still need to cleanup some files after

We may want to consider devendoring Versioneer, but that can be follow on work

Generally this seems like a reasonable approach. Think the version constraint on nvidia-ml-py will do most of the work for us

In terms of reworking things in conda-forge, yes we will want to fix lines that you have indicated in nvidia-ml-py. We can change the pynvml constraint to >=12

Likely we will also want a repodata patch to fix old versions to work as well. Would want to check with Lawrence since he knows which versions have the bug fixes we need

rjzamora · 2024-11-22T14:39:54Z

Think we are using Versioneer here. We would want to follow these steps as if we were configuring versioneer from scratch

I think I already did this back in August (maybe?). Was this in response to my question about import pynvml; pynvml.__version__ returning an error? If so, I don't think there is much we can do about that within this repo. When pynvml becomes a meta package, importing pynvml in python will not touch anything in this repo.

rjzamora · 2024-11-22T15:21:15Z

Update: I removed versioneer to simplify the repo a bit. I also made some small test changes. However, there is still one failing test (Cause is the NVML_P2P_CAPS_INDEX_READ problem reported in this dev forum).

setup.py

jakirkham · 2024-11-22T20:46:34Z

Think we are using Versioneer here. We would want to follow these steps as if we were configuring versioneer from scratch

I think I already did this back in August (maybe?). Was this in response to my question about import pynvml; pynvml.__version__ returning an error? If so, I don't think there is much we can do about that within this repo. When pynvml becomes a meta package, importing pynvml in python will not touch anything in this repo.

Yes

Update: I removed versioneer to simplify the repo a bit. I also made some small test changes. However, there is still one failing test (Cause is the NVML_P2P_CAPS_INDEX_READ problem reported in this dev forum).

Removing versioneer may have helped with this problem

pynvml_utils/tests/test_nvml.py

.gitattributes

rjzamora · 2024-11-22T20:49:48Z

Removing versioneer may have helped with this problem

Unfortunately not, because the problem is in nvidia-ml-py

requirements.txt

jakirkham · 2024-11-22T20:51:16Z

Should we go ahead and convert this to pyproject.toml? That would help us consolidate several different files into one file with the relevant metadata

Co-authored-by: jakirkham <[email protected]>

.gitattributes

rjzamora · 2024-11-25T16:26:55Z

Should we go ahead and convert this to pyproject.toml? That would help us consolidate several different files into one file with the relevant metadata

@jakirkham - Would this be easy to you to contribute? I don't personally deal much with build/packaging details, so I'd need to learn a bit about toml files first :)

jakirkham

Thanks Rick! 🙏

Had some feedback on the changes so far

jakirkham · 2024-11-25T22:53:51Z

requirements.txt

-pytest>=3.6,<7.0
-pytest-runner
-pytest-cov


Think we should still capture these somewhere as they are used for testing

jakirkham · 2024-11-25T22:54:59Z

setup.py

 here = path.abspath(path.dirname(__file__))

 # Get the long description from the README file
 with open(path.join(here, 'README.md'), encoding='utf-8') as f:
    long_description = f.read()

 setup(name='pynvml',
-      version=versioneer.get_version(),
-      cmdclass=versioneer.get_cmdclass(),
-      python_requires='>=3.6',


Think it is unwise to drop this minimum. It is unlikely their code works on anything older than Python 3.6 (possibly it even needs a newer version). The code we still ship at least needs Python 3.6 (possibly newer depending on what we test with)

jakirkham · 2024-11-25T22:56:50Z

setup.py

@@ -2,18 +2,15 @@
 from os import path
 from io import open

-import versioneer
-
 here = path.abspath(path.dirname(__file__))

 # Get the long description from the README file
 with open(path.join(here, 'README.md'), encoding='utf-8') as f:
    long_description = f.read()

 setup(name='pynvml',


Given the simplicity of this code, think we can move this to a pyproject.toml, which will simplify and deduplicate dependency handling, metadata (like the README), etc.

Okay, I gave it a go. My knowledge in this area is limited, so feel free to suggest/push changes.

jakirkham

Thanks Rick! 🙏

Generally this looks good. Had a few suggestions below

pyproject.toml

jakirkham · 2024-11-26T21:30:00Z

requirements.txt

 pip>=9.0.1
-pytest>=3.6,<7.0
+pytest>=3.6
 pytest-runner
 pytest-cov
+nvidia-ml-py>=12.0.0,<13.0.0a0


As this is now in the test requirements in pyproject.toml, think we can drop this

Is the suggestion to drop the requirements.txt file altogether? That seems fine to me. I only left it because it is used in the Dockerfile (though I don't personally know if anyone is using that).

Update: I removed requirements.txt now that the Dockerfile doesn't need it.

setup.py

pyproject.toml

jakirkham · 2024-11-26T21:54:19Z

docker/Dockerfile

    pip install --no-cache-dir -r /app/requirements.txt && \
    rm -rf ~/.cache/pip

-ADD ./pynvml /app/pynvml
-ADD ./setup.py /app/
-ADD ./setup.cfg /app/
+ADD ./pynvml_utils /app/pynvml_utils
 ADD ./README.md /app/
-ADD ./PKG-INFO /app/
-ADD ./MANIFEST.in /app/
 ADD ./help_query_gpu.txt /app/
 ADD docker/entrypoint.sh /docker-entrypoint.sh


Think we can consolidate this to one ADD of the whole directory and then use pip install .

Alternatively this might be a good time to clean this up if it is unneeded

Okay, gave this a try.

Co-authored-by: jakirkham <[email protected]>

jakirkham

Thanks Rick! 🙏

Think this is pretty close to done

Had a few cleanup suggestions below

MANIFEST.in

pyproject.toml

docker/Dockerfile

Co-authored-by: jakirkham <[email protected]>

jakirkham · 2024-11-27T00:59:21Z

Thanks Rick! 🙏

This looks reasonable to me

@kenhester could you please take a look as well?

rjzamora · 2024-11-27T23:08:49Z

Note: Ken confirmed offline that we are good to move forward with this.

remove pynvml module - make pynvml a meta package instead

4a8f847

rjzamora added 4 commits November 22, 2024 06:53

add upper bound to nvidia-ml-py dependency

0ff599a

fix smi import

c38c3e3

remove versioneer and set version to 12.0

d3527c6

update some tests

3ce78b3

rjzamora marked this pull request as ready for review November 22, 2024 15:21

rjzamora changed the title ~~[WIP] Remove pynvml module and convert to a "meta package"~~ Remove pynvml module and convert to a "meta package" Nov 22, 2024

rjzamora self-assigned this Nov 22, 2024

rjzamora added 3 commits November 22, 2024 07:41

update readme

11cd254

update .gitattributes

d18c220

tweak warning

553f086