-
Notifications
You must be signed in to change notification settings - Fork 11
Support Python 3.9 for PyIceberg #637
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
Conversation
new_axis0 = pd.TimedeltaIndex( | ||
index_data, name=head_axis0.name, unit=head_axis0.unit | ||
) | ||
elif isinstance(head_axis0, pd.Index): |
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.
pd.Index
needs to be moved last in the class matching case since it's the super class of others.
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, thanks @ehsantn !
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.
Looks good, thanks @ehsantn !
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 Ehsan!
- numba 0.61.2 | ||
- numba >=0.60 |
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.
Should this be numba >=0.60,<0.62
here and the other places we specify numba version?
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.
Specifying upper version limits causes trouble for dependency solvers and is not great practice. We are less sensitive to Numba versions at this point and will upgrade quickly if necessary.
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.
Yeah, I'd just be worried that a numba upgrade would break our old package versions forcing users to upgrade their bodo version but if new numba versions are unlikely to cause issues, makes sense
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #637 +/- ##
==========================================
+ Coverage 65.42% 65.71% +0.28%
==========================================
Files 176 179 +3
Lines 63691 65436 +1745
Branches 8915 9262 +347
==========================================
+ Hits 41672 42998 +1326
- Misses 19397 19790 +393
- Partials 2622 2648 +26 |
Changes included in this PR
Adds back Python 3.9 support since PyIceberg's Poetry setup requires Python 3.9: apache/iceberg-python#2167
The main changes are avoiding structured pattern matching and adapting type annotations.
Testing strategy
Tested Iceberg (local, S3 Tables), dataframe library unit tests, and NYC Taxi JIT locally with Python 3.9.
Nightly run passed except some flaky issues (rerunning to be sure): https://dev.azure.com/bodo-inc/Bodo/_build/results?buildId=25853&view=results
User facing changes
Python 3.9 packages will be available.
Checklist
[run CI]
in your commit message.