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

[Data] Unpin pyarrow from test-requirements #39290

Merged
merged 7 commits into from
Sep 8, 2023

Conversation

scottjlee
Copy link
Contributor

@scottjlee scottjlee commented Sep 6, 2023

Why are these changes needed?

When installing ray, we found that pyarrow 6.0.1 is installed, while our default requirements.txt and other requirements files specify pyarrow >= 6.0.1 or higher. So, we try removing the pinned pyarrow==6.0.1 in test-requirements.txt to see if this is still necessary. We expect that if this is no longer needed, removing this pin will allow the resolved pyarrow version to be higher than Arrow 6, since pyarrow is already installed from the default requirements.txt, and there is a manual Arrow version override for Data tests already. This will allow users to fully utilize critical pyarrow 7+ features out of the gate, without having to manually install a higher version of Arrow.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Signed-off-by: Scott Lee <sjl@anyscale.com>
Copy link
Contributor

@krfricke krfricke left a comment

Choose a reason for hiding this comment

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

Hi @scottjlee,

unfortunately this doesn't have an effect on the actual installed dependencies as python/requirements_compiled.txt is still pinning pyarrow.

Instead, after applying your change, you should run ./ci/ci.sh compile_pip_dependencies on a Linux machine with Python 3.8 to re-compile the dependencies.

(Ideally you can also do the same for Python 3.7 with ./ci/ci.sh compile_pip_dependencies requirements_compiled_py37.txt).

Let me know if you need any support with that.

Kai Fricke added 3 commits September 7, 2023 11:48
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke
Copy link
Contributor

krfricke commented Sep 7, 2023

I've updated the PR with a new compiled requirements file. So far it looks good, we'll just have to see if all tests are passing.

@krfricke
Copy link
Contributor

krfricke commented Sep 7, 2023

Potentially we need to re-add a pyarrow 6 test, depending on if we want to continue to test support for it.

Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke krfricke marked this pull request as ready for review September 7, 2023 16:49
Copy link
Contributor

@krfricke krfricke left a comment

Choose a reason for hiding this comment

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

I've updated the PR and tests seem to be passing. Would be good to get another pair of eyes on cc @matthewdeng

Signed-off-by: Kai Fricke <kai@anyscale.com>
Copy link
Contributor

@matthewdeng matthewdeng left a comment

Choose a reason for hiding this comment

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

Looks good assuming this was generated in clean environment.

@matthewdeng matthewdeng merged commit 5867f32 into ray-project:master Sep 8, 2023
45 of 106 checks passed
krfricke pushed a commit to krfricke/ray that referenced this pull request Sep 8, 2023
Signed-off-by: Scott Lee <sjl@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Co-authored-by: matthewdeng <matt@anyscale.com>
matthewdeng added a commit to matthewdeng/ray that referenced this pull request Sep 8, 2023
Signed-off-by: Scott Lee <sjl@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Co-authored-by: matthewdeng <matt@anyscale.com>
GeneDer added a commit that referenced this pull request Sep 9, 2023
* [Data] Unpin pyarrow from test-requirements (#39290)

Signed-off-by: Scott Lee <sjl@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Co-authored-by: matthewdeng <matt@anyscale.com>

* Update requirements_compiled.txt

* Update test-requirements.txt

---------

Signed-off-by: Scott Lee <sjl@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Co-authored-by: Scott Lee <scottjlee@users.noreply.github.com>
Co-authored-by: matthewdeng <matt@anyscale.com>
Co-authored-by: Gene Der Su <e870252314@gmail.com>
jimthompson5802 pushed a commit to jimthompson5802/ray that referenced this pull request Sep 12, 2023
Signed-off-by: Scott Lee <sjl@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Co-authored-by: matthewdeng <matt@anyscale.com>
Signed-off-by: Jim Thompson <jimthompson5802@gmail.com>
vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023
Signed-off-by: Scott Lee <sjl@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Co-authored-by: matthewdeng <matt@anyscale.com>
Signed-off-by: Victor <vctr.y.m@example.com>
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.

None yet

3 participants