-
Notifications
You must be signed in to change notification settings - Fork 6
deps: Drop support for Python 3.7 and 3.8 #337
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
Removes support for Python 3.7 and 3.8, establishing Python 3.9 as the new minimum supported version. This change involves: - Updating `python_requires` and classifiers in `setup.py`. - Modifying Python versions in `noxfile.py` (default, unit tests, system tests) and ensuring constraint file logic remains correct. - Updating the GitHub Actions workflow (`unittest.yml`) matrix, runner, and coverage job version. - Deleting constraint files for Python 3.7 and 3.8 (`testing/constraints-3.7.txt`, `testing/constraints-3.8.txt`). - Removing Kokoro sample configuration directories (`.kokoro/samples/python3.7/`, `.kokoro/samples/python3.8/`). - Updating supported version mentions in `README.rst`. - Removing 3.7 and 3.8 from the `ALL_VERSIONS` list in `samples/snippets/noxfile.py`.
Just a suggestion, maybe use |
Conventional Commit points at this page as reference for prefixes: 'build', If Google also uses deps, then sure. |
Wow, I suggested it because we used it in the commit for python-bigquery. Apparently it's not part of convetional commits, but it worked. Edit: |
The purpose of that class is to support older versions of pandas (and numpy, I believe). If it's not covered, it's likely because we aren't testing / supporting such versions anymore. Please delete dead code rather than mark as |
db_dtypes/pandas_backports.py
Outdated
@import_default("pandas.core.arrays._mixins") | ||
class NDArrayBackedExtensionArray( | ||
pandas.core.arrays.base.ExtensionArray | ||
): # pragma: NO COVER |
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.
If these isn't used, then we should delete it. Per previous:
@import_default("pandas.core.arrays._mixins", pandas_release < (1, 3))
This was only used in pandas 1.2 tests, so it's not used anymore that the pandas minimum version is bumped to 1.5.3.
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 am struggling with this. I would like to suggest that this specific issue is tangential to the scope of this PR and should be handled after this PR is merged as part of a new refactoring PR.
Regarding the comment: "If it's not covered, it's likely because we aren't testing / supporting such versions anymore"
It appears that we never had tests for this class, so the lack of tests seems to be unrelated. I do not know why coverage did not flag this, but it flags it now, hence the pragma as a temporary solution to allow the work to proceed.
I know it was marked as < pandas 1.3 in the preceding comment.
In those comments it was also noted it could be dropped when NDArrayBackedExtension
is added to pandas (presumably in 1.5/2.0) but that doesn't appear to have happened.
We use end up referring to this class as a dependency via subclassing in
in BaseDateTimeArray
. BaseDateTimeArray
is subclassed in TimeArray
and DateArray
. BaseDateTimeArray
thus carries with it a number of attributes and methods from NDArrayBackedExtension
that are relied upon across a plethora of tests throughout our test suite in multiple files (including unit, compliance, and prerelease tests)
If I remove this class and attempt to subclass BaseDateTimeArray
on something like pandas.api.extensions.ExtensionArray
, ExtensionArray
does not have all the attributes and methods that are included in NDArrayBackedExtension
and so assorted tests continue to fail.
Even if I replace those specific missing attributes and methods, that does not solve all the problems because ExtensionArray
is somehow different and we still have other tests that also fail (I stopped there in my attempts to resolve this comment).
It feels like a catch-22, if I take this out we end up going down a rabbit hole to try and figure all the nuances of why this breaks so many things. That task is tangential to the scope of this PR which is to remove the dependencies on 3.7 and 3.8. It will also likely greatly increase the size and complexity of this PR.
If I try to create a separate PR to just refactor this portion of the code before we deprecate 3.7 and 3.8, I will likely have to do at least some deprecation of 3.7 and 3.8 in the new PR to ensure that we are comparing apples to apples and capturing all the failure modes, which means we end up duplicating a subset of the work from this PR.
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 know it was marked as < pandas 1.3 in the preceding comment.
Right. It was only used in such environments. Now such an environment should not be possible. We can remove these backports since they aren't needed with the bump in mimumum versions.
In those comments it was also noted it could be dropped when NDArrayBackedExtension is added to pandas (presumably in 1.5/2.0) but that doesn't appear to have happened.
It is there, it's just not part of the public interface. It would be good to revisit that but at this point the interface is quite stable and we've mostly moved on to pyarrow-based extension arrays instead.
methods that are included in NDArrayBackedExtension and so assorted tests continue to fail.
Why would you change the superclass? The backport thing was just to move some code around. Just use the real class directly.
If I try to create a separate PR to just refactor this portion of the code before we deprecate 3.7 and 3.8,
Just delete the unused code. It's not a refactor.
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 deleted the unused code and adjusted the signature for BaseDateTimeArray
class to use the pandas.core.array._mixins.NDArrayBackedExtensionArray
directly.
@@ -47,15 +46,6 @@ | |||
_NP_BOX_DTYPE = "datetime64[us]" | |||
|
|||
|
|||
# To use JSONArray and JSONDtype, you'll need Pandas 1.5.0 or later. With the removal | |||
# of Python 3.7 compatibility, the minimum Pandas version will be updated to 1.5.0. | |||
if packaging.version.Version(pandas.__version__) >= packaging.version.Version("1.5.0"): |
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.
From the comments, it seems removing Python 3.7 could ensure pandas >= 1.5.0, and thus ensure JSONArray and JSONDtype to be available. Do we still need _determine_all()
?
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.
Removed.
tests/unit/test__init__.py
Outdated
assert "JSONDtype" in result | ||
assert "JSONArray" in result | ||
assert "JSONArrowType" in result |
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.
Nit: these don't seem necessary if we already verified that the sets are equal.
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.
Removed.
tests/unit/test_json.py
Outdated
|
||
import db_dtypes.json # noqa: F401 | ||
|
||
assert True, "Module re-import completed without error, except block likely worked." |
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 the string after the comma is part of the raised error if the assertion failed, but the wording describes the expected behavior.
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.
Removed.
tests/unit/test_json.py
Outdated
|
||
|
||
@pytest.fixture | ||
def cleanup_json_module_for_reload(): |
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 understand that this fixture makes sure db_dtypes.json is removed before any unit test that might want to import it with a clean slate, and we have added two unit tests verifying that it works. But I don't see this fixture being used in any tests. Should we add it to where we intend to use it?
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.
If we really use this fixture, we probably can get rid of all the # pragma: NO COVER
inside.
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.
Removed.
db_dtypes/pandas_backports.py
Outdated
# TODO: use public API once NDArrayBackedExtensionArray is added to the | ||
# pandas. |
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.
It is in pandas. That's what import_default
is doing. It's importing from pandas.core.arrays._mixins
. We don't need this class anymore. Just use pandas.core.arrays._mixins.NDArrayBackedExtensionArray
directly.
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.
Used the suggested class directly.
db_dtypes/pandas_backports.py
Outdated
# Also: Right now none of this is tested in test_pandas_backports. | ||
# Temporarily marking this as # pragma: NO COVER just to see how this | ||
# affects unittest coverage | ||
@import_default("pandas.core.arrays._mixins") | ||
class NDArrayBackedExtensionArray( | ||
pandas.core.arrays.base.ExtensionArray | ||
): # pragma: NO COVER |
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.
If we aren't changing the imports in other files, just add the class here like this:
# Also: Right now none of this is tested in test_pandas_backports. | |
# Temporarily marking this as # pragma: NO COVER just to see how this | |
# affects unittest coverage | |
@import_default("pandas.core.arrays._mixins") | |
class NDArrayBackedExtensionArray( | |
pandas.core.arrays.base.ExtensionArray | |
): # pragma: NO COVER | |
NDArrayBackedExtensionArray = pandas.core.arrays._mixins.NDArrayBackedExtensionArray |
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.
We only referenced NDArrayBackedExtensionArray
in a single class: BaseDateTimeArray
.
I updated that class signature directly.
This branch is generated automagically via AI as an experiment.
Requires a comprehensive human review and comment.
Do not merge until approved by an authorized reviewer.
Removes support for Python 3.7 and 3.8, establishing Python 3.9 as the new minimum supported version.
This change involves:
python_requires
and classifiers insetup.py
.noxfile.py
(default, unit tests, system tests) and ensuring constraint file logic remains correct.unittest.yml
) matrix, runner, and coverage job version.testing/constraints-3.7.txt
,testing/constraints-3.8.txt
)..kokoro/samples/python3.7/
,.kokoro/samples/python3.8/
).README.rst
.ALL_VERSIONS
list insamples/snippets/noxfile.py
.BEGIN_COMMIT_OVERRIDE
deps: Drop support for Python 3.7 and 3.8
END_COMMIT_OVERRIDE