AIPCC-10079: Allow overriding extract_info_from_wheel_file #912
Conversation
Xaenalt
force-pushed
the
AIPCC-10079
branch
4 times, most recently
from
90ea79e to
95cf4ec
Compare
This happens due to an unfortuante way that TensorFlow is built. Since it uses Bazel to build and package the wheel, it doesn't have the usual infrastructure one uses to build variant wheels. Due to that, a single repo is used to build wheels with different names. In Fromager's case, this means we specify the req as `tensorflow` but in reality will get `tensorflow_cpu` or `tensorflow_rocm`, etc. This is how they create them upstream, and to match the requirements from notebooks that the wheel naming convention remains the same, we have to override the check that the wheel name matches the req in extract_info_from_wheel_file(). To achieve this, I made it overridable like other functions and provided a default_extract_info_from_wheel_file() implemetation to preserve existing behavior. I expect TensorFlow will be one of the only users of this override. Signed-off-by: Sean Pryor <[email protected]>
| return (dist_name, dist_version, build_tag, wheel_tags) | ||
|
|
||
|
|
||
| def extract_info_from_wheel_file( |
tiran
left a comment
tiran
left a comment
There was a problem hiding this comment.
We have multiple packages that build different distributions from the same sources:
- tensorflow (CUDA, CPU, ROCm)
- cupy-cuda (12x, 13x)
- opencv-python (currently we build only headless)
How does this PR address the problem that users request tensorflow-cpu or tensorflow-rocm in their requirements?
|
In this case, the req tensorflow builds tensorflow-cpu, tensorflow, tensorflow-rocm, but because the req doesn't match the filename, it fails that sanity check. The other option I can try is to create package plugins for tensorflow_cpu and tensorflow_rocm and have them reuse the code in tensorflow, but I didn't want to duplicate a lot of logic when there was a simpler way. If that's the preferred approach I can do that though |
|
It looks to me like the original function was mixing concerns. Extracting the info and validating it are two different things. If we split those, I think it would be more obvious that we need either a validation hook or, better, a config option for the expected wheel name to use in the core validation function. |
Typically, our users are expecting the same behavior as with pip/uv and same names as on upstream PyPI. They will have
We could add an option to tell Fromager that $ ln -sr overrides/settings/tensorflow.yaml overrides/settings/tensorflow_cpu.yaml
$ ln -sr overrides/settings/tensorflow.yaml overrides/settings/tensorflow_rocm.yaml[project.entry-points."fromager.project_overrides"]
tensorflow = "package_plugins.tensorflow"
tensorflow_cpu = "package_plugins.tensorflow"
tensorflow_rocm = "package_plugins.tensorflow"Our downstream linter will not accept |
|
Oh! That'd be perfect, that'd even simplify the other patch too |
4 participants
This happens due to an unfortuante way that TensorFlow is built. Since it uses Bazel to build and package the wheel, it doesn't have the usual infrastructure one uses to build variant wheels. Due to that, a single repo is used to build wheels with different names. In Fromager's case, this means we specify the req as
tensorflowbut in reality will gettensorflow_cpuortensorflow_rocm, etc. This is how they create them upstream, and to match the requirements from notebooks that the wheel naming convention remains the same, we have to override the check that the wheel name matches the req in extract_info_from_wheel_file().To achieve this, I made it overridable like other functions and provided a default_extract_info_from_wheel_file() implemetation to preserve existing behavior. I expect TensorFlow will be one of the only users of this override.