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

Drop Python 3.6 support #4460

Merged
merged 24 commits into from Jul 26, 2022
Merged

Drop Python 3.6 support #4460

merged 24 commits into from Jul 26, 2022

Conversation

mariosasko
Copy link
Contributor

@mariosasko mariosasko commented Jun 8, 2022

Remove the fallback imports/checks in the code needed for Python 3.6 and update the requirements/CI files. Also, use Python types for the NumPy dtype wherever possible to avoid deprecation warnings in newer NumPy versions.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 8, 2022

The documentation is not available anymore as the PR was closed or merged.

@mariosasko mariosasko marked this pull request as ready for review June 16, 2022 17:12
@mariosasko
Copy link
Contributor Author

I've disabled the test_dummy_dataset_serialize_s3 tests in the Linux CI to avoid the failures (these tests only fail on Windows in 3.6). These failures are unrelated to this PR's changes, and I would like to address this in a new PR.

@lhoestq lhoestq mentioned this pull request Jun 23, 2022
3 tasks
@@ -3119,7 +3119,7 @@ def test_pickle_dataset_after_transforming_the_table(in_memory, method_and_param


@pytest.mark.skipif(
os.name == "nt" and (os.getenv("CIRCLECI") == "true" or os.getenv("GITHUB_ACTIONS") == "true"),
os.name in ["nt", "posix"] and (os.getenv("CIRCLECI") == "true" or os.getenv("GITHUB_ACTIONS") == "true"),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this ? The test currently passes on main on linux

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't on Python 3.7. Notice the test_dummy_dataset_serialize_s3 failure in one of the previous test

Copy link
Member

Choose a reason for hiding this comment

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

Ok. This test is almost never run then, Maybe let's remove it completely in another PR, and make another one to make sure S3 export works as expected. Maybe we can use the mockfs fixture defined in #4724 instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This issue seems related to getmoto/moto#4750, but neither bumping moto to 3.0.0 (CI runs forever) nor pinning responses to 0.16 (the serialization fails) helps, so I agree we can disable these tests for now and replace them in another PR with the mockfs fixture

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.

Thanks :) not sure why torchaudio fails to decode/resample in the CI though (it uses the latest version 0.12.0)

src/datasets/utils/py_utils.py Show resolved Hide resolved
src/datasets/packaged_modules/text/dataset_infos.json Outdated Show resolved Hide resolved
@mariosasko
Copy link
Contributor Author

This comment explains the issue with MP3 decoding in torchaudio in the latest release (supports Python 3.7+). I fixed CI by pinning torchaudio to <0.12.0. Another way to fix this issue is by installing ffmpeg with conda or using the unofficial GH action. But I don't think it's worth making CI more complex, considering we can wait for the soundfile release, which should bring MP3 decoding, and drop the torchaudio dependency then.

@julien-c
Copy link
Member

Yay for dropping Python 3.6!

@mariosasko mariosasko requested a review from lhoestq July 26, 2022 14:08
@mariosasko
Copy link
Contributor Author

I think we can merge in this state. Also, if an env has Python version < 3.7 installed, we raise a warning, so I don't think we even need to create (and pin) an issue to notify the contributors of this change.

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.

Thanks ! LGTM :)

@mariosasko mariosasko merged commit 75e6b74 into main Jul 26, 2022
@mariosasko mariosasko deleted the drop-python36 branch July 26, 2022 19:04
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

4 participants