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

Update python to 3.8 #571

Merged
merged 4 commits into from
Apr 18, 2024
Merged

Conversation

qubvel
Copy link
Member

@qubvel qubvel commented Apr 10, 2024

The latest versions of datasets and transformers require Python 3.8. Currently, CI and a package have Python 3.7 as a requirement, leading to tests failing (#569). I was able to reproduce the failing tests with the same dependencies installed during the CI, probably it's worth updating the package's Python requirement to Python 3.8 too.

I have updated Python to 3.8 in CI to see if it became green.

@qubvel
Copy link
Member Author

qubvel commented Apr 10, 2024

Now, other tests are failing

  1. confidence interval computed with scipy.stats.bootstrap, test pass for scipy <= 1.9.3, but fails on newer versions of scipy >= 1.10.0. Probably the test is not reliable enough.
  2. The distributed metric test fails, looks similar to Evaluate fails to acquire lock interrupting single-node multi-GPU training #540, several breakages due to recent datasets  #542. Test is passing with filelock <= 3.11.0.

I would appreciate any thoughts @lhoestq

@lhoestq
Copy link
Member

lhoestq commented Apr 15, 2024

Cool, thansk for updating :) Maybe we should also update setup.py to require python >= 3.8 as well. The scipy test can surely be fixed using numpy.testing.assert_almost_equal or something like that, if you want to fix it in this PR

I'm a bit less sure about the filelock issue, we can leave that for later.

@qubvel
Copy link
Member Author

qubvel commented Apr 15, 2024

Regarding scipy, adding tolerance would be not enough, because the current confidence interval in tests (0.3333, 0.6666), but after scipy update we have (0.3355, 1.0).
We can either update the test range and fix the scipy above 1.10.0 or just fix scipy under 1.9.3.

@lhoestq
Copy link
Member

lhoestq commented Apr 15, 2024

Ah yes indeed. We should align with the latest scipy

@qubvel
Copy link
Member Author

qubvel commented Apr 16, 2024

Ok, I fixed scipy version, the only failing test is for the distributed metrics.
Do you have any ideas on how to fix it without downgrading filelock or should we add a constraint to the version?
@lhoestq maybe you can tag anyone else who can help?

@lhoestq lhoestq mentioned this pull request Apr 18, 2024
Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

LGTM ! I opened #578 that hopefully fixes the last test

@lhoestq lhoestq merged commit 7a18c55 into huggingface:main Apr 18, 2024
4 of 6 checks passed
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

2 participants