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

fix: for backwards compatibility, expose legacy retry imports #577

Merged
merged 9 commits into from
Jan 29, 2024

Conversation

vchudnov-g
Copy link
Contributor

@vchudnov-g vchudnov-g commented Dec 20, 2023

Fixes #575

BEGIN_COMMIT_OVERRIDE
chore: for backwards compatibility, expose legacy retry imports in the refactored retry logic
END_COMMIT_OVERRIDE

@vchudnov-g vchudnov-g requested review from a team as code owners December 20, 2023 00:06
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Dec 20, 2023
@vchudnov-g vchudnov-g marked this pull request as draft December 20, 2023 00:06
Comment on lines 34 to 42
import datetime # noqa: F401
import functools # noqa: F401
import logging # noqa: F401
import random # noqa: F401
import sys # noqa: F401
import time # noqa: F401
import inspect # noqa: F401
import warnings # noqa: F401
from typing import Any, Callable, TypeVar, TYPE_CHECKING # noqa: F401
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were having a discussion today as to whether these imports of standard libraries are appropriate. The correct thing to do is for users to import them directly, and not throughgoogle.api_core.retry. However, if they have been using these imports incorrectly (à la import google.api_core.retry.logging), then us not providing these symbols here is technically a breaking change: they'll have to change their calling code. Should we require them to do that, so that they import things correctly, or do we adhere to a strict/pedantic definition of breaking change, where the user's code really does not have to change at all, even if they are using a non-recommended pattern?

Part of the issue extends beyond just this refactoring: what does this mean for future maintenance work that happens to remove an import (import logging, let's say) but keeps our surface the same. Should we continue to expose the import we no longer need just so we don't break users who unwisely were using import google.api_core.retry.logging instead of import logging? That doesn't seem like a good idea, but breaking users still makes me uncomfortable.

I have not yet been able to find a policy document talking about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Searches like this one on GitHub and this one on Google suggest that there aren't many repos, if any, importing standard packages in non-standard ways, so maybe we're fine to to not expose them here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a separate discussion, we agreed to not expose standard imports transitively if we don't need them in our implementation.

I think it would be good if the Python community spelled this out somewhere, but that's a separate issue.

@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: m Pull request size is medium. labels Jan 29, 2024
@vchudnov-g vchudnov-g marked this pull request as ready for review January 29, 2024 21:20
@vchudnov-g vchudnov-g changed the title fix(WIP): for backwards compatibility, expose legacy retry imports fix: for backwards compatibility, expose legacy retry imports Jan 29, 2024
tests/unit/retry/test_retry_imports.py Outdated Show resolved Hide resolved
@vchudnov-g vchudnov-g merged commit cb777a1 into main Jan 29, 2024
27 checks passed
@vchudnov-g vchudnov-g deleted the retries-include branch January 29, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

continue exposing some retry imports for backwards compatibility
2 participants