You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/06/21 20:47:46 UTC
[GitHub] [beam] AnandInguva opened a new pull request, #21968: Replace distutils with supported modules.
AnandInguva opened a new pull request, #21968:
URL: https://github.com/apache/beam/pull/21968
Distutils is planned to be removed from Python 3.12. For Python 3.10, 3.11 there will be a deprecation warning when distutils is imported. To avoid the depreciation warnings, Replace distutils with supported modules.
distutils issue: https://github.com/apache/beam/issues/19419
Python 3.10: https://github.com/apache/beam/issues/21458
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r905468048
##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -54,14 +54,14 @@
import shutil
import sys
import tempfile
-from distutils.version import StrictVersion
from typing import Callable
from typing import List
from typing import Optional
from typing import Tuple
from urllib.parse import urlparse
import pkg_resources
+from pkg_resources import parse_version
Review Comment:
We need setuptools to build the `sdist`. I eliminated the usage of distutils almost in the code base except for the `DistutilErrorr` which is used in `setup.py`. To import that error, we just need to import it after importing `setuptools`
For `StrictVersion`, `parse_version` was used because it follows PEP-440 and we need to sort the version of pip and some other modules in our code. `parse_version` comes from `pkg_resources` module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1162336777
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ryanthompson591 commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1163128649
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1162390817
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r918205440
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
Review Comment:
Add:
```
# It is recommended to import setuptools prior to importing distutils to avoid
# using legacy behavior from distutils.
# https://setuptools.readthedocs.io/en/latest/history.html#v48-0-0
```
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
+from distutils.errors import DistutilsError # pylint: disable=wrong-import-order
Review Comment:
> Now we don't need to try to maintain setuptools>=60.0.0 requirement.
Sounds like we need v48 or higher though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1162336782
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r904230272
##########
.github/workflows/build_wheels.yml:
##########
@@ -240,7 +240,7 @@ jobs:
working-directory: apache-beam-source
env:
CIBW_BUILD: ${{ matrix.os_python.python }}
- CIBW_BEFORE_BUILD: pip install cython
+ CIBW_BEFORE_BUILD: pip install cython && pip install --upgrade setuptools
Review Comment:
Looking at the code base, there was never a version pinned to the setuptools. it was `pip install setuptools` most of the times[1](https://github.com/apache/beam/blob/6e25012b8d1cf1e690cda580cd00ae035b733324/sdks/python/container/run_generate_requirements.sh#L58).
The image used to build github wheels use a very old version of setuptools. I guess it would be on positive note if we update setuptools IIUC.
@tvalentyn what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] yeandy commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
yeandy commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r904226659
##########
.github/workflows/build_wheels.yml:
##########
@@ -240,7 +240,7 @@ jobs:
working-directory: apache-beam-source
env:
CIBW_BUILD: ${{ matrix.os_python.python }}
- CIBW_BEFORE_BUILD: pip install cython
+ CIBW_BEFORE_BUILD: pip install cython && pip install --upgrade setuptools
Review Comment:
Any dangers with always upgrading `setuptools`? I wonder if there a reason this wasn't done before?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r905284124
##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -54,14 +54,14 @@
import shutil
import sys
import tempfile
-from distutils.version import StrictVersion
from typing import Callable
from typing import List
from typing import Optional
from typing import Tuple
from urllib.parse import urlparse
import pkg_resources
+from pkg_resources import parse_version
Review Comment:
I read more about the integration of distutils with setuptools. Setuptools has been maintaining a copy of disutils. If we import setuptools, it also imports `setuptools/distutils` or replaces(if already imported) `stdlib/distutils` with `setuptools/distutils`.
In this PR, i made changes to reflect
1. setuptools to be imported before distutils. [[1](https://github.com/pypa/setuptools/blob/669be5ef1d93e2fa088cc5d1dfd999e0db61abcf/_distutils_hack/__init__.py#L18)]
2. Replace the usage of distutils with alternatives. For example, for StricVersion, we can replace it with `pkg_resources.parse_version` since both follow [PEP-440](https://peps.python.org/pep-0440/)
3. Update setuptools while building wheels and building python containers since these images use old version of setuptools
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] yeandy commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
yeandy commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r906244818
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
+from distutils.errors import DistutilsError # pylint: disable=wrong-import-order
Review Comment:
Ah, that makes sense. Now we don't need to try to maintain `setuptools>=60.0.0` requirement. Thanks for the clarification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r905284124
##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -54,14 +54,14 @@
import shutil
import sys
import tempfile
-from distutils.version import StrictVersion
from typing import Callable
from typing import List
from typing import Optional
from typing import Tuple
from urllib.parse import urlparse
import pkg_resources
+from pkg_resources import parse_version
Review Comment:
I read more about the integration of distutils with setuptools. Setuptools has been maintaining a copy of disutils. If we import setuptools, it also imports `setuptools/distutils` or replaces(if already imported) `stdlib/distutils` with `setuptools/distutils`.
In this PR, i made changes to reflect
1. setuptools to be imported before distutils. [[1](https://github.com/pypa/setuptools/blob/main/_distutils_hack/__init__.py)]
2. Replace the usage of distutils with alternatives. For example, for StricVersion, we can replace it with `pkg_resources.parse_version` since both follow [PEP-440](https://peps.python.org/pep-0440/)
3. Update setuptools while building wheels and building python containers since these images use old version of setuptools
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r905462760
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
+from distutils.errors import DistutilsError # pylint: disable=wrong-import-order
Review Comment:
So the `distutils` is present in two places. In Python standard library and also in setuptools. `setuptools` adopted `stdlib` implementation of `distutils` and the `stdlib distutils` is marked for deprecation in Python 3.10 and removal for Python 3.12.
Now, in order to use `distutils`, we can just import `setuptools`, which implicitly imports its version of `distutils`. it is recommended to import `distutils` after `setuptools`, otherwise we might use `stdlib distutils` which is deprecated and no longer maintained
I read a bit and found the information above. Now we don't need the `BaseError` which requires `setuptools>=60.0.0`. We just need to reorder the imports wrt `setuptools` and `distutils`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r905462760
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
+from distutils.errors import DistutilsError # pylint: disable=wrong-import-order
Review Comment:
So the `distutils` is present in two places. Python standard library and also in setuptools. `setuptools` adopted stdlib implementation of `distutils` and the `stdlib distutils` is marked for deprecation in Python 3.10 and removal for Python 3.12.
Now, in order to use `distutils`, we can just import `setuptools`, which implicitly imports its version of `distutils`. As of now, there are two versions of `distutils`, it is recommended to import `distutils` after `setuptools`.
I read a bit and found the information above. Now we don't need the `BaseError` which requires `setuptools>=60.0.0`. We just need to reorder the imports wrt `setuptools` and `distutils`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r905285459
##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -54,14 +54,14 @@
import shutil
import sys
import tempfile
-from distutils.version import StrictVersion
from typing import Callable
from typing import List
from typing import Optional
from typing import Tuple
from urllib.parse import urlparse
import pkg_resources
+from pkg_resources import parse_version
Review Comment:
Now, its not a strict requirement to have setuptools >= 60.0.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r904230272
##########
.github/workflows/build_wheels.yml:
##########
@@ -240,7 +240,7 @@ jobs:
working-directory: apache-beam-source
env:
CIBW_BUILD: ${{ matrix.os_python.python }}
- CIBW_BEFORE_BUILD: pip install cython
+ CIBW_BEFORE_BUILD: pip install cython && pip install --upgrade setuptools
Review Comment:
Looking at the code base, there was never a version pinned to the setuptools. it was `pip install setuptools` most of the times[[1](https://github.com/apache/beam/blob/6e25012b8d1cf1e690cda580cd00ae035b733324/sdks/python/container/run_generate_requirements.sh#L58)].
The image used to build github wheels use a very old version of setuptools. I guess it would be on positive note if we update setuptools IIUC.
@tvalentyn what do you think?
##########
.github/workflows/build_wheels.yml:
##########
@@ -240,7 +240,7 @@ jobs:
working-directory: apache-beam-source
env:
CIBW_BUILD: ${{ matrix.os_python.python }}
- CIBW_BEFORE_BUILD: pip install cython
+ CIBW_BEFORE_BUILD: pip install cython && pip install --upgrade setuptools
Review Comment:
Looking at the code base, there was never a version pinned to the setuptools. it was `pip install --upgrade setuptools` most of the times[[1](https://github.com/apache/beam/blob/6e25012b8d1cf1e690cda580cd00ae035b733324/sdks/python/container/run_generate_requirements.sh#L58)].
The image used to build github wheels use a very old version of setuptools. I guess it would be on positive note if we update setuptools IIUC.
@tvalentyn what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r904910320
##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -54,14 +54,14 @@
import shutil
import sys
import tempfile
-from distutils.version import StrictVersion
from typing import Callable
from typing import List
from typing import Optional
from typing import Tuple
from urllib.parse import urlparse
import pkg_resources
+from pkg_resources import parse_version
Review Comment:
how do we make sure users have setuptools >= 60 installed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r929651999
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1195172218
PTAL @tvalentyn
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1164711439
PTAL @yeandy @tvalentyn. updated the description
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r904230272
##########
.github/workflows/build_wheels.yml:
##########
@@ -240,7 +240,7 @@ jobs:
working-directory: apache-beam-source
env:
CIBW_BUILD: ${{ matrix.os_python.python }}
- CIBW_BEFORE_BUILD: pip install cython
+ CIBW_BEFORE_BUILD: pip install cython && pip install --upgrade setuptools
Review Comment:
Looking at the code base, there was never a version pinned to the setuptools. it was `pip install --upgrade setuptools` most of the times[[1](https://github.com/apache/beam/blob/6e25012b8d1cf1e690cda580cd00ae035b733324/sdks/python/container/run_generate_requirements.sh#L58)].
The image used to build wheels in github actions use a very old version of setuptools. I guess it would be on positive note if we update setuptools IIUC.
@tvalentyn what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1195749077
Oops! I thought all the checks passed. I will make a new PR and fix the lint error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1162336788
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1162520783
R: @yeandy @ryanthompson591
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1162521235
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1163457617
R: @tvalentyn
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1162415537
# [Codecov](https://codecov.io/gh/apache/beam/pull/21968?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#21968](https://codecov.io/gh/apache/beam/pull/21968?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (80ae1d4) into [master](https://codecov.io/gh/apache/beam/commit/316c969b9c373177986ee0fbc794ed2887244328?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (316c969) will **increase** coverage by `0.00%`.
> The diff coverage is `33.33%`.
```diff
@@ Coverage Diff @@
## master #21968 +/- ##
=======================================
Coverage 74.01% 74.01%
=======================================
Files 702 702
Lines 92845 92848 +3
=======================================
+ Hits 68716 68722 +6
+ Misses 22864 22861 -3
Partials 1265 1265
```
| Flag | Coverage Δ | |
|---|---|---|
| python | `83.59% <33.33%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/21968?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...on/apache\_beam/examples/complete/juliaset/setup.py](https://codecov.io/gh/apache/beam/pull/21968/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vZXhhbXBsZXMvY29tcGxldGUvanVsaWFzZXQvc2V0dXAucHk=) | `0.00% <0.00%> (ø)` | |
| [...s/python/apache\_beam/runners/portability/stager.py](https://codecov.io/gh/apache/beam/pull/21968/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9zdGFnZXIucHk=) | `85.54% <50.00%> (ø)` | |
| [...hon/apache\_beam/runners/worker/bundle\_processor.py](https://codecov.io/gh/apache/beam/pull/21968/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvYnVuZGxlX3Byb2Nlc3Nvci5weQ==) | `93.42% <0.00%> (-0.25%)` | :arrow_down: |
| [sdks/python/apache\_beam/runners/common.py](https://codecov.io/gh/apache/beam/pull/21968/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9jb21tb24ucHk=) | `88.68% <0.00%> (-0.25%)` | :arrow_down: |
| [setup.py](https://codecov.io/gh/apache/beam/pull/21968/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2V0dXAucHk=) | `0.00% <0.00%> (ø)` | |
| [...thon/apache\_beam/ml/inference/pytorch\_inference.py](https://codecov.io/gh/apache/beam/pull/21968/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWwvaW5mZXJlbmNlL3B5dG9yY2hfaW5mZXJlbmNlLnB5) | `0.00% <0.00%> (ø)` | |
| [sdks/python/apache\_beam/ml/inference/base.py](https://codecov.io/gh/apache/beam/pull/21968/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWwvaW5mZXJlbmNlL2Jhc2UucHk=) | `95.37% <0.00%> (+0.08%)` | :arrow_up: |
| [...ks/python/apache\_beam/runners/worker/sdk\_worker.py](https://codecov.io/gh/apache/beam/pull/21968/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvc2RrX3dvcmtlci5weQ==) | `89.09% <0.00%> (+0.15%)` | :arrow_up: |
| [...eam/runners/portability/fn\_api\_runner/execution.py](https://codecov.io/gh/apache/beam/pull/21968/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL2V4ZWN1dGlvbi5weQ==) | `93.08% <0.00%> (+0.64%)` | :arrow_up: |
| [sdks/python/apache\_beam/io/localfilesystem.py](https://codecov.io/gh/apache/beam/pull/21968/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vbG9jYWxmaWxlc3lzdGVtLnB5) | `91.72% <0.00%> (+0.75%)` | :arrow_up: |
| ... and [1 more](https://codecov.io/gh/apache/beam/pull/21968/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/21968?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/21968?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [316c969...80ae1d4](https://codecov.io/gh/apache/beam/pull/21968?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r905462760
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
+from distutils.errors import DistutilsError # pylint: disable=wrong-import-order
Review Comment:
So the `distutils` is present in two places. In Python standard library and also in setuptools. `setuptools` adopted `stdlib` implementation of `distutils` and the `stdlib distutils` is marked for deprecation in Python 3.10 and removal for Python 3.12.
Now, in order to use `distutils`, we can just import `setuptools`, which implicitly imports its version of `distutils`. As of now, there are two versions of `distutils`, it is recommended to import `distutils` after `setuptools`.
I read a bit and found the information above. Now we don't need the `BaseError` which requires `setuptools>=60.0.0`. We just need to reorder the imports wrt `setuptools` and `distutils`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r904907981
##########
.github/workflows/build_wheels.yml:
##########
@@ -240,7 +240,7 @@ jobs:
working-directory: apache-beam-source
env:
CIBW_BUILD: ${{ matrix.os_python.python }}
- CIBW_BEFORE_BUILD: pip install cython
+ CIBW_BEFORE_BUILD: pip install cython && pip install --upgrade setuptools
Review Comment:
should be safe to upgrade pip and setuptools.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r918205852
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
+from distutils.errors import DistutilsError # pylint: disable=wrong-import-order
Review Comment:
> Now we don't need to try to maintain setuptools>=60.0.0 requirement.
Sounds like we need v48 or higher though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r918205440
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
Review Comment:
Add:
# It is recommended to import setuptools prior to importing distutils to avoid
# using legacy behavior from distutils.
# https://setuptools.readthedocs.io/en/latest/history.html#v48-0-0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1162511667
Run Java PreCommit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1162336776
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r905468048
##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -54,14 +54,14 @@
import shutil
import sys
import tempfile
-from distutils.version import StrictVersion
from typing import Callable
from typing import List
from typing import Optional
from typing import Tuple
from urllib.parse import urlparse
import pkg_resources
+from pkg_resources import parse_version
Review Comment:
We need setuptools to build the `sdist`. I eliminated the usage of `distutils` almost in the code base except for the `DistutilErrorr` which is used in `setup.py`. To import that error, we just need to import it after importing `setuptools`
For `StrictVersion`, `parse_version` was used because it follows PEP-440 and we need to sort the version of pip and some other modules in our code. `parse_version` comes from `pkg_resources` module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] yeandy commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
yeandy commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r905435086
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
+from distutils.errors import DistutilsError # pylint: disable=wrong-import-order
Review Comment:
So basically, as long as setuptools is always imported before distutils, we can still use `DistutilsError` because it's maintained separately inside `setuptools`?
##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -54,14 +54,14 @@
import shutil
import sys
import tempfile
-from distutils.version import StrictVersion
from typing import Callable
from typing import List
from typing import Optional
from typing import Tuple
from urllib.parse import urlparse
import pkg_resources
+from pkg_resources import parse_version
Review Comment:
Am I understanding this correctly: Since `setuptools` has its own copy of `distutils`, this means we can still use the original `distutils` logic (provided we make your changes), right?
And you said we can replace `StrictVersion` with the alternative `parse_version`. Besides minimizing usage of `distutils`, are there any other benefits of using `parse_version`? Maybe in the future, when don't want to use `setuptools'` copy of `distutils` anymore, it would be easier to refactor any remaining usage of `distutils`?
Finally, I saw setuptools' [warning](https://github.com/pypa/setuptools/blob/main/_distutils_hack/__init__.py) that "To avoid these issues, avoid "using distutils directly, ensure that setuptools is installed in the traditional way (e.g. not an editable install), and/or make sure that setuptools is always imported before distutils." To me, it feels safer (and easier) to manger to eliminate using `distutils` entirely than to maintain our codebase such that `setuptools` is always imported before `distutils`.
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
+from distutils.errors import DistutilsError # pylint: disable=wrong-import-order
Review Comment:
And we would rather keep this because `DistutilsError` is more meaningful than your previously proposed usage of `setuptools.errors.BaseError`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r918661906
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
+from distutils.errors import DistutilsError # pylint: disable=wrong-import-order
Review Comment:
Yep. We can just leave `setuptools` without specifying the version
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1195745973
sorry, I accidentally merged this while lint check was failing, so I rolled it back immediately. Please submit a new PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r918205852
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
+from distutils.errors import DistutilsError # pylint: disable=wrong-import-order
Review Comment:
> Now we don't need to try to maintain setuptools>=60.0.0 requirement.
Sounds like we need v48 or higher though. Note that it cannot be explicitly requested: https://github.com/pypa/setuptools/issues/1004
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn merged pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
tvalentyn merged PR #21968:
URL: https://github.com/apache/beam/pull/21968
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r905284124
##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -54,14 +54,14 @@
import shutil
import sys
import tempfile
-from distutils.version import StrictVersion
from typing import Callable
from typing import List
from typing import Optional
from typing import Tuple
from urllib.parse import urlparse
import pkg_resources
+from pkg_resources import parse_version
Review Comment:
I read more about the integration of distutils with setuptools. Setuptools has been maintaining a copy of disutils. If we import setuptools, it also imports `setuptools/distutils` or replaces(if already imported) `stdlib/distutils` with `setuptools/distutils`.
In this PR, i made changes to reflect
1. setuptools to be imported before distutils. [[1](https://github.com/pypa/setuptools/blob/669be5ef1d93e2fa088cc5d1dfd999e0db61abcf/_distutils_hack/__init__.py#L18)]
2. Replace the usage of distutils with alternatives. For example, for `StricVersion`, we can replace it with `pkg_resources.parse_version` since both follow [PEP-440](https://peps.python.org/pep-0440/)
3. Update setuptools while building wheels and building python containers since these images use old version of setuptools
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1163288125
> Can you clarify the part where you said that `BaseError` is using "DistutilsError under the hood, even though `DistutilsError` is being deprecated?
>
> And what was the takeaway from testing the `setuptools` versions (where I was able to run `from setuptools.errors import DistutilsError` in setuptools versions is 49.2.1, 59.6.0, 60.8.1, but not for 62.0.0 or 62.1.0)? Was it to ensure overlapping coverage as we transition to `BaseError`?
According to [PEP632](https://peps.python.org/pep-0632/#:~:text=Table%20of%20Contents-,Abstract,-The%20distutils%20module), setuptools has integrated a complete copy of the `distutils` from version [60.0.0](https://setuptools.pypa.io/en/latest/deprecated/distutils-legacy.html). From this [release](https://github.com/pypa/setuptools/blob/9288c6f3f039bf51f997a99ae8766ed02ed92cda/setuptools/errors.py#L40), `BaseError` is defined to `BaseError = _distutils.DistutilsError`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21968:
URL: https://github.com/apache/beam/pull/21968#discussion_r905462760
##########
sdks/python/setup.py:
##########
@@ -30,9 +28,15 @@
from pkg_resources import DistributionNotFound
from pkg_resources import get_distribution
from pkg_resources import normalize_path
+from pkg_resources import parse_version
from pkg_resources import to_filename
from setuptools import Command
+# distutils from stdlib is deprecated from python 3.10.
+# setuptools has been maintaining a copy of distutils and importing
+# setuptools will replace stdlib/distutils with setuptools/distutils.
+from distutils.errors import DistutilsError # pylint: disable=wrong-import-order
Review Comment:
So the `distutils` is present in two places. In Python standard library and also in setuptools. `setuptools` adopted `stdlib` implementation of `distutils` and the `stdlib distutils` is marked for deprecation in Python 3.10 and removal for Python 3.12.
Now, in order to use `distutils`, we can just import `setuptools`, which implicitly imports its version of `distutils`. it is recommended to import `distutils` after `setuptools`, otherwise we might use `stdlib distutils`
I read a bit and found the information above. Now we don't need the `BaseError` which requires `setuptools>=60.0.0`. We just need to reorder the imports wrt `setuptools` and `distutils`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1163903630
Run Python PreCommit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #21968: Replace distutils with supported modules.
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #21968:
URL: https://github.com/apache/beam/pull/21968#issuecomment-1162336787
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org