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

vdk-core: Support for 3.11 #1395

Merged
merged 3 commits into from
Dec 5, 2022
Merged

vdk-core: Support for 3.11 #1395

merged 3 commits into from
Dec 5, 2022

Conversation

antoniivanov
Copy link
Collaborator

@antoniivanov antoniivanov commented Dec 2, 2022

With the GA release of Python 3.11 a month ago it's time to officially support it.
That means we need to start testing against Python 3.11. See release notes at https://docs.python.org/3/whatsnew/3.11.html

This change is adding tests (and changes related to make the tests run) for 3.11 for vdk-core.

Subsequent changes will add that for all other components that support multiple versions of python

While troubleshooting fixed the path to tests.xml so that it shows properly in the test tab - https://gitlab.com/vmware-analytics/versatile-data-kit/-/pipelines/712924440/test_report and to be able to download the XML test report

Part 1 of #1397

Testing Done: this PR pipeline.

Signed-off-by: Antoni Ivanov [email protected]

@antoniivanov antoniivanov force-pushed the person/aivanov/upgrade-311 branch 2 times, most recently from efe0966 to fc572f0 Compare December 2, 2022 13:18
@antoniivanov antoniivanov changed the title vdk-all: Support for 3.11 vdk-core: Support for 3.11 Dec 2, 2022
@antoniivanov antoniivanov marked this pull request as ready for review December 2, 2022 13:19
@antoniivanov antoniivanov linked an issue Dec 2, 2022 that may be closed by this pull request
@antoniivanov antoniivanov force-pushed the person/aivanov/upgrade-311 branch 2 times, most recently from 0d419eb to a42171f Compare December 2, 2022 14:53
@antoniivanov antoniivanov force-pushed the person/aivanov/upgrade-311 branch 3 times, most recently from 8dd4992 to b081704 Compare December 4, 2022 23:01
@antoniivanov antoniivanov enabled auto-merge (squash) December 4, 2022 23:03
@ivakoleva
Copy link
Contributor

ivakoleva commented Dec 5, 2022

This change is adding a tests for 3.11 across all official python components that test against multiple versions of python.

Could you enumerate those changed components?

I've clarified the description. It's about vdk-core only. Subsequent changes will "upgrade" other components.

IK: It includes the utils also, that is a separate artifact. So why not porting it in its entirety, for atomicity purposes and git history readability?

@ivakoleva
Copy link
Contributor

Could you link to the Python 3.11 changelog? Have you reviewed it, is there any concerns we should know about? If not, should we consider it to be the default version soon?

@ivakoleva
Copy link
Contributor

Is there any impact for the job builders, for example

RUN apk add --no-cache --repository http://dl-cdn.alpinelinux.org/alpine/v3.10/main python3=3.7.10-r0 py3-pip \

@antoniivanov
Copy link
Collaborator Author

Could you link to the Python 3.11 changelog? Have you reviewed it, is there any concerns we should know about? If not, should we consider it to be the default version soon?

https://docs.python.org/3/whatsnew/3.11.html

I've seen no obvious concerns. But I'd wait for 3.11.1 to be released just in case.

@antoniivanov
Copy link
Collaborator Author

Is there any impact for the job builders, for example

RUN apk add --no-cache --repository http://dl-cdn.alpinelinux.org/alpine/v3.10/main python3=3.7.10-r0 py3-pip \

Those are independent things. Here we are only adding explicit tests that cover support for 3.11 and fixes things to work with 3.11 (if there are any). We still support 3.7+

The linked script is part of the builder job implementation. It uses 3.10 and that's fine. It's just for the builder system job

@ivakoleva
Copy link
Contributor

I have added an issue comment regarding the expectations of this change. Most probably this information should be added within the PR description as well #1397 (comment)

With the GA release of Python 3.11 a month ago it's time to officially
support it.
That means we need to start testing against Python 3.11.

This change is adding a tests for 3.11 across all official python
components that test against multiple versions of python.

Testing Done: this PR pipeline.

Signed-off-by: Antoni Ivanov <[email protected]>
With the GA release of Python 3.11 a month ago it's time to officially
support it.
That means we need to start testing against Python 3.11.

This change is adding a tests for 3.11 across all official python
components that test against multiple versions of python.

Testing Done: this PR pipeline.

Signed-off-by: Antoni Ivanov <[email protected]>
@antoniivanov antoniivanov force-pushed the person/aivanov/upgrade-311 branch from ec2d7ca to d9e4a0a Compare December 5, 2022 09:26
@antoniivanov antoniivanov removed a link to an issue Dec 5, 2022
@ivakoleva
Copy link
Contributor

Is there any impact for the job builders, for example

RUN apk add --no-cache --repository http://dl-cdn.alpinelinux.org/alpine/v3.10/main python3=3.7.10-r0 py3-pip \

Those are independent things. Here we are only adding explicit tests that cover support for 3.11 and fixes things to work with 3.11 (if there are any). We still support 3.7+

The linked script is part of the builder job implementation. It uses 3.10 and that's fine. It's just for the builder system job

Well it seems to me it used to rely on python latest (3.10 before this PR change), along with language level pinned to 3.7. Unless you upgrade it with the scope of this 3.10->3.11 transition, then why choosing to break the latest supported sources+custom language level convention? It seems, by not changing it you introduce complexity that may be redundant. If any problem with porting the builders, we may consider an additional effort.

@antoniivanov
Copy link
Collaborator Author

antoniivanov commented Dec 5, 2022

Well it seems to me it used to rely on python latest (3.10 before this PR change), along with language level pinned to 3.7. Unless you upgrade it with the scope of this 3.10->3.11 transition, then why choosing to break the latest supported sources+custom language level convention? It seems, by not changing it you introduce complexity that may be redundant. If any problem with porting the builders, we may consider an additional effort.

This is not part of the scope of this PR. This PR is about vdk-core . I've added a comment in #1397

Copy link
Contributor

@ivakoleva ivakoleva left a comment

Choose a reason for hiding this comment

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

I'm up for a live review meeting if needed

@antoniivanov antoniivanov merged commit 3af304f into main Dec 5, 2022
@antoniivanov antoniivanov deleted the person/aivanov/upgrade-311 branch December 5, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants