Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated pip and packaging versions to work with free-threading packages #2514

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vfdev-5
Copy link
Contributor

@vfdev-5 vfdev-5 commented Dec 19, 2024

We had an issue to install jaxlib with bazel when running the following command (using rules_python v0.39):

          bazel test \
              --repo_env=HERMETIC_PYTHON_VERSION=3.13-ft \
              --repo_env=JAX_NUM_GENERATED_CASES=$JAX_NUM_GENERATED_CASES \
              --repo_env=JAX_ENABLE_X64=$JAX_ENABLE_X64 \
              --repo_env=JAX_SKIP_SLOW_TESTS=$JAX_SKIP_SLOW_TESTS \
              --repo_env=PYTHON_GIL=$PYTHON_GIL \
              --repo_env=TSAN_OPTIONS="halt_on_error=1" \
              --//jax:build_jaxlib=false \
              --nocache_test_results \
              --test_output=all \
              //tests:cpu_tests

According to @vam-google, this was due to old pip/packaging versions. We updated them and this helped to make work the whole building/testing pipeline: jax-ml/jax#24898
So, we would like to upstream the patch: https://github.com/jax-ml/jax/pull/24898/files#diff-e3dc8d7d2bf5d057f95b86bcff7360b6c99fa1f458882fd112b58da4aceb53e4

@vfdev-5
Copy link
Contributor Author

vfdev-5 commented Dec 19, 2024

@aignas can you help with updating the tests to take new versions into account?

@aignas
Copy link
Collaborator

aignas commented Dec 20, 2024

Yeah, not sure about the fix, but it does seem that the suggested change is breaking the reproducibility of the compiled requirement files. I am wondering though if this is just a test setup issue.

Unfortunately, I have no time to work on this.

@vfdev-5 vfdev-5 force-pushed the updated-pip-packaging-versions branch from d76ba9a to c86039f Compare December 21, 2024 01:12
copybara-service bot pushed a commit to google/tsl that referenced this pull request Dec 21, 2024
…ding support.

The same change has been sent upstream as bazelbuild/rules_python#2514

Forked from openxla/xla#20723 for merging.

PiperOrigin-RevId: 708471912
copybara-service bot pushed a commit to openxla/xla that referenced this pull request Dec 21, 2024
…ding support.

The same change has been sent upstream as bazelbuild/rules_python#2514

Forked from #20723 for merging.

PiperOrigin-RevId: 708471912
@vfdev-5 vfdev-5 force-pushed the updated-pip-packaging-versions branch from c86039f to 0dcfd8c Compare December 21, 2024 01:54
@@ -9,8 +9,6 @@
pip==22.3.1 \
--hash=sha256:65fd48317359f3af8e593943e6ae1506b66325085ea64b706a998c6e83eeaf38 \
--hash=sha256:908c78e6bc29b676ede1c4d57981d490cb892eb45cd8c214ab6298125119e077
# via -r requirements.in
Copy link
Contributor Author

@vfdev-5 vfdev-5 Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aignas The recent pip==24.3.1 (vs 24.0) now adds the annotation as an absolute path vs relative path previously:

- # via -r requirements.in
+ # via -r /var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/5b986f10e8407a028a0eb165b28c6c27/execroot/_main/bazel-out/k8-fastbuild/bin/requirements.update.runfiles/_main/requirements.in

The idea is to remove annotations in the tests only to match requirements_lock.txt on the reproducibility process.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the CI of ours needs to be fixed, that means that this PR will break users that depend on annotations and asking all of the users add --no-annotate to their setups is not something that we can do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any ideas how to make this less BC breaking?
On the other hand, I think it would be a tiny BC breaking change in rules python 1.0 compared to others. This would also enable the use of more recent tools instead of being stuck with old versions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vfdev-5 vfdev-5 force-pushed the updated-pip-packaging-versions branch from 0dcfd8c to c0d36d4 Compare December 21, 2024 02:12
copybara-service bot pushed a commit to google/tsl that referenced this pull request Dec 21, 2024
…ding support.

The same change has been sent upstream as bazelbuild/rules_python#2514

Forked from openxla/xla#20723 for merging.

PiperOrigin-RevId: 708471912
copybara-service bot pushed a commit to openxla/xla that referenced this pull request Dec 21, 2024
…ding support.

The same change has been sent upstream as bazelbuild/rules_python#2514

Forked from #20723 for merging.

PiperOrigin-RevId: 708471912
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Dec 21, 2024
…ding support.

The same change has been sent upstream as bazelbuild/rules_python#2514

Forked from openxla/xla#20723 for merging.

PiperOrigin-RevId: 708471912
@vfdev-5 vfdev-5 force-pushed the updated-pip-packaging-versions branch 2 times, most recently from 6701c97 to 9b406af Compare December 23, 2024 09:29
# match[2] is "/absolute/path/to/requirements*.in"
return match[1] + Path(match[2]).name

content = pattern.sub(replace_abspath_with_name, content)
Copy link
Contributor Author

@vfdev-5 vfdev-5 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aignas is this something like what you thought about in this message: #2514 (comment) ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually thinking that the absolute_path_prefix replacement stopped working due to some reason and that we should fix the absolute_path_prefix matching, but it seems that in reality it is something else.

Would it be possible to have the absolute_path_prefix contain the actual absolute path prefix that works here?

Copy link
Contributor Author

@vfdev-5 vfdev-5 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not totally sure what does absolute_path_prefix replacement, running the test

cd tests/integration/compile_pip_requirements

echo '' > requirements_lock.txt && \
    bazel run //:requirements.update && \
    git diff --exit-code

on main, I have the following values in the vars:

resolved_requirements_file: /tmp/jax/rules_python/tests/integration/compile_pip_requirements/requirements_lock.txt
absolute_path_prefix: /tmp/jax/rules_python/tests/integration/compile_pip_requirements/ 

However, requirements lock content does not contain these paths (even before content = content.replace(absolute_path_prefix, "") line):

# This file is autogenerated by pip-compile with Python 3.9                                                
# by the following command:                  
#                                                                                                          
#    bazel run //:requirements.update                                                                      
#                                                                                                          
                                                                                                           
# The following packages are considered to be unsafe in a requirements file:                               
pip==22.3.1 \
    --hash=sha256:65fd48317359f3af8e593943e6ae1506b66325085ea64b706a998c6e83eeaf38 \
    --hash=sha256:908c78e6bc29b676ede1c4d57981d490cb892eb45cd8c214ab6298125119e077
    # via -r requirements.in
setuptools==65.6.3 \
    --hash=sha256:57f6f22bde4e042978bcd50176fdb381d7c21a9efa4041202288d3737a0c6a54 \
    --hash=sha256:a7620757bf984b58deaf32fc8a4577a9bbc0850cf92c20e1ce41c38c19e5fb75
    # via -r requirements_extra.in

So, nothing is replaced with content = content.replace(absolute_path_prefix, "").

If you can share more details on what was the intention of content = content.replace(absolute_path_prefix, ""), I can try to figure out whether this could work with newer pip.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that on Windows things work a little bit differently and it could be that this is only needed there. I can't fully remember the exact reasons for the code, but I do think that trying to use that might be useful.

Copy link
Contributor Author

@vfdev-5 vfdev-5 Dec 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me recap the problem.

New pip==24.3.1 (vs 24.0) now adds the annotation as an absolute path vs relative path previously:

- # via -r requirements.in
+ # via -r /root/.cache/bazel/_bazel_root/a76a5e2851b1f2cc4ad9886a6a6aacc6/execroot/_main/bazel-out/k8-fastbuild/bin/requirements.update.runfiles/_main/requirements.in

In the test, for example, tests/integration/compile_pip_requirements, we use piptools compile as following:

argv = ['--output-file=requirements_lock.txt', 'requirements.txt', '--resolver=backtracking', '--allow-unsafe', '--generate-hashes']
cli(argv)

Files requirements.txt and requirements_lock.txt are and will be located in /root/.cache/bazel/_bazel_root/a76a5e2851b1f2cc4ad9886a6a6aacc6/execroot/_main/bazel-out/k8-fastbuild/bin/requirements.update.runfiles/_main/ folder.

The content of requirements.txt is:

-r requirements.in

After the execution, pip produces the lock file with the following content:

#
# This file is autogenerated by pip-compile with Python 3.9
# by the following command:
#
#    bazel run //:requirements.update
#

# The following packages are considered to be unsafe in a requirements file:
pip==22.3.1 \
    --hash=sha256:65fd48317359f3af8e593943e6ae1506b66325085ea64b706a998c6e83eeaf38 \
    --hash=sha256:908c78e6bc29b676ede1c4d57981d490cb892eb45cd8c214ab6298125119e077
    # via -r /root/.cache/bazel/_bazel_root/a76a5e2851b1f2cc4ad9886a6a6aacc6/execroot/_main/bazel-out/k8-fastbuild/bin/requirements.update.runfiles/_main/requirements.in
setuptools==65.6.3 \
    --hash=sha256:57f6f22bde4e042978bcd50176fdb381d7c21a9efa4041202288d3737a0c6a54 \
    --hash=sha256:a7620757bf984b58deaf32fc8a4577a9bbc0850cf92c20e1ce41c38c19e5fb75
    # via -r /root/.cache/bazel/_bazel_root/a76a5e2851b1f2cc4ad9886a6a6aacc6/execroot/_main/bazel-out/k8-fastbuild/bin/requirements.update.runfiles/_main/requirements_extra.in

And the test expects to see

...
# via -r requirements.in
...
# via -r requirements_extra.in

On my linux machine, absolute_path_prefix is set to /tmp/jax/rules_python/tests/integration/compile_pip_requirements/ where the test source code is located. It is computed from resolved_requirements_file which is again points to the original requirements file: /tmp/jax/rules_python/tests/integration/compile_pip_requirements/requirements_lock.txt

I'm not sure if we can efficiently use absolute_path_prefix without breaking something else (like test on Windows).

We can probably fetch the absolute path of the source requirement.txt file <-> argv[1] and remove it from all lines # via -r .... What do you think ? I implemented this here: d52fc23

Concerning my code in this PR, it may not always work as source file can be anything and not only requirements*.in as I'm regexping now...

@vfdev-5 vfdev-5 force-pushed the updated-pip-packaging-versions branch 4 times, most recently from a3249de to c9307ef Compare December 23, 2024 10:52
copybara-service bot pushed a commit to openxla/xla that referenced this pull request Dec 23, 2024
…ding support.

The same change has been sent upstream as bazelbuild/rules_python#2514

Forked from #20723 for merging.

PiperOrigin-RevId: 708471912
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Dec 23, 2024
…ding support.

The same change has been sent upstream as bazelbuild/rules_python#2514

Forked from openxla/xla#20723 for merging.

PiperOrigin-RevId: 708471912
copybara-service bot pushed a commit to openxla/xla that referenced this pull request Dec 23, 2024
…ding support.

The same change has been sent upstream as bazelbuild/rules_python#2514

Forked from #20723 for merging.

PiperOrigin-RevId: 708471912
aignas added a commit to aignas/rules_python that referenced this pull request Dec 24, 2024
This is reusing a bit of code used in `evaluate_markers` and makes use
of the RECORD files in the `whl` files that we use to extract whls in
`whl_library`. This should be merged before bazelbuild#2514 to avoid any cache
invalidation issues downstream.

Fixes bazelbuild#2468
@vfdev-5 vfdev-5 force-pushed the updated-pip-packaging-versions branch 4 times, most recently from 22e1a4e to 033bd41 Compare December 24, 2024 13:18
@vfdev-5 vfdev-5 force-pushed the updated-pip-packaging-versions branch from 0f921dc to 32ae8dc Compare December 24, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants