Add wheel builds#901
Conversation
Co-authored-by: Lawrence Mitchell <[email protected]>
Co-authored-by: Lawrence Mitchell <[email protected]>
Co-authored-by: Lawrence Mitchell <[email protected]>
Co-authored-by: Lawrence Mitchell <[email protected]>
This reverts commit c595e93.
jakirkham
left a comment
jakirkham
left a comment
There was a problem hiding this comment.
Thanks Vyas for all the hard work here! 🙏
Had some thoughts on simplifying the version handling below
| ftext = f.read() | ||
| major = re.findall("^#define.*UCP_API_MAJOR.*", ftext, re.MULTILINE) | ||
| minor = re.findall("^#define.*UCP_API_MINOR.*", ftext, re.MULTILINE) | ||
| versioneer.get_versions = get_versions |
There was a problem hiding this comment.
Am curious why we are overwriting the version function here? It seems simpler just to not use versioneer at all in this case
There was a problem hiding this comment.
I'll defer to @wence- here, but the answer here is related to his answer to your other query about why not directly use the version: this hack ensures that we get the right version encoded in _version.py so that ucp.__version__ is correct when users import and use it.
There was a problem hiding this comment.
Yes, it's all part of the same dance.
The other approach would be to rewrite _version.py in the wheel-building process to fill in the version by hand. But this is kind of what versioneer does any, so this monkeypatch is simpler.
| @@ -56,6 +63,10 @@ def get_ucp_version(): | |||
| cmdclass = versioneer.get_cmdclass(cmdclass) | |||
There was a problem hiding this comment.
Could we condition this on whether the version was set externally and skip this if so?
There was a problem hiding this comment.
Given the discussions around versioneer.get_version I think it's more appropriate to leave all of this unconditional and make the versioneer monkey patch the only conditional change.
| # name="ucx-py" + os.getenv("RAPIDS_PY_WHEEL_CUDA_SUFFIX", default=""), | ||
| ext_modules=ext_modules, | ||
| cmdclass=cmdclass, | ||
| version=versioneer.get_version(), |
There was a problem hiding this comment.
Maybe we should just pass the version here if known
There was a problem hiding this comment.
The rationale for this is as for the approach we took in dask-cuda. I copy the description:
The GIT_DESCRIBE_TAG and VERSION_SUFFIX environment variables are used
to control the name and version of the created conda/pypi package.
They should, however, not be used to control the version of the
installed package by overriding the versioneer cmdclass since that
leaves an unmodified _version.py file in the installed package
directory. A consequence is that the version reported by
dask_cuda.version is "0+unknown".
We cannot always use the versioneer-provided cmdclass unmodified since
PEP440 specifically forbids PyPI from accepting packages that have local
version identifiers (as used by versioneer). To get around this, when setup.py
detects it is building a PyPI package (GIT_DESCRIBE_TAG is in the environment),
patch the version returned from versioneer with a PyPI-compatible one.
Specifically. If we don't use versioneer here, then the dynamic code in __init__.py which imports _version and calls get_versions() will report nonsense for wheel packages.
| cibw-before-build: "cp {package}/.github/build-scripts/patch-wheel.sh /patch-wheel.sh" | ||
| # Also rewrite the name in the pyproject.toml file. | ||
| # TODO: Eventually this needs to be done more generically. | ||
| cibw-before-build: "cp {package}/.github/build-scripts/patch-wheel.sh /patch-wheel.sh && sed -i 's/name = \"ucx-py\"/name = \"ucx-py-cu11\"/g' {package}/pyproject.toml" |
There was a problem hiding this comment.
Was thinking we could also capture this in a patch we apply as part of the build. No strong feelings about that or this approach though
There was a problem hiding this comment.
Let's stick to this approach for now and punt determining an "optimal" long-term solution for the next release. I don't know if we want to store multiple patches in the repo since that will create multiple sources of truth (the GHA workflow will also need to have access to the current CUDA version, for instance). I think it will be cleaner in the long run to use this sort of scripting approach that reacts to the values of inputs to the workflow.
|
@jakirkham do you have further concerns here about the version management (or anything else)? |
|
In the interest of getting things in for code freeze I'm going to go ahead and merge this. If there are any outstanding concerns about the version handling we can address that in the next release. |
|
@gpucibot merge |
|
Ah I guess I don't have write access to this repository. @wence- or @pentschev could you please merge this? Thanks! |
|
@gpucibot merge |
This PR adds support for building wheels of ucx-py. Authors: - Vyas Ramasubramani (https://github.com/vyasr) - Benjamin Zaitlen (https://github.com/quasiben) - Sevag H (https://github.com/sevagh) Approvers: - Lawrence Mitchell (https://github.com/wence-) - Joseph (https://github.com/jolorunyomi) URL: rapidsai#901
6 participants
This PR adds support for building wheels of ucx-py.