-
Notifications
You must be signed in to change notification settings - Fork 370
Remove Python 3.9 support #2554
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
base: main
Are you sure you want to change the base?
Conversation
@Fokko Thanks! I will update this. |
@Fokko I’ve updated the changes. I had one query, is it okay to work on an issue if no PR has been linked or mentioned for it yet? |
Yes, for sure. In Iceberg, everyone is free to work on any issue, even when someone else claims it. We often see folks picking up issues but getting into time constraints and not being able to finish the work; therefore, we don't assign it to anyone. |
pyproject.toml
Outdated
ray = [ | ||
{ version = "==2.10.0", python = "<3.9", optional = true }, | ||
{ version = ">=2.10.0,<=2.44.0", python = ">=3.9", optional = true }, | ||
{ version = ">=2.10.0,<=2.44.0", python = ">=3.10", optional = true }, | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can simplify this to:
ray = { version = ">=2.10.0,<=2.44.0", optional = true }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! sure
pyproject.toml
Outdated
ray = [ | ||
{ version = "==2.10.0", python = "<3.9", optional = true }, | ||
{ version = ">=2.10.0,<=2.44.0", python = ">=3.9", optional = true }, | ||
{ version = ">=2.10.0,<=2.44.0", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get rid of the brackets []
, since the list only has one entry :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for correcting me, I will update this
@Fokko I’ve applied the changes based on your suggestions. Please let me know if any further improvements are needed. |
@Aniketsy It looks like we're one |
yes, i will update . |
@Fokko I’m not sure if I added the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are a few more mentions of 3.9
- .github/workflows/pypi-build-artifacts.yml
- .github/workflows/svn-build-artifacts.yml
- mkdocs/docs/contributing.md
i usually would do |
2e0cc40
to
e66c5aa
Compare
@kevinjqliu I've applied the changes. |
mkdocs/docs/contributing.md
Outdated
For the type annotation the types from the `Typing` package are used. | ||
|
||
PyIceberg offers support from Python 3.9 onwards, we can't use the [type hints from the standard collections](https://peps.python.org/pep-0585/). | ||
PyIceberg offers support from Python 3.10 onwards, we can't use the [type hints from the standard collections](https://peps.python.org/pep-0585/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can revert this change.
https://peps.python.org/pep-0585/ is supported by 3.9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah looks like this was find/replaced when we removed 3.8
857abd0#diff-c0ad53549c03c8e66a0e3574698ce24dfa722b54ca7a2fdfcb8d62ff0343ab73R205-R206
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can remove this paragraph entirely then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! i will update
looks like there a conflict, could you rebase this PR with main?
You will likely have to regen the poetry.lock file again
|
9c83e18
to
721aac3
Compare
@kevinjqliu I’ve removed the paragraph as suggested. Please take a look at the changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
lets wait for more folks to chime in on the devlist thread before merging
https://lists.apache.org/thread/z3dhxx2wc0dhvgq9js5ktbqf4ggsn4z8
oops looks like poetry doesnt like that theres no upperlimit, we can use |
This is fantastic! If we do remove Python 3.9, there's a bunch of new typing features in 3.10 that we can now introduce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome! Thank you so much for doing this
#2534
Removed python 3.9 support.
Please let me know if my approach or fix needs any improvements . I’m open to feedback and happy to make changes based on suggestions.
Thankyou !