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

chore: add async_rest extra for async rest dependencies #701

Merged
merged 7 commits into from
Sep 30, 2024

Conversation

parthea
Copy link
Collaborator

@parthea parthea commented Sep 20, 2024

This PR adds an extra for async_rest which captures the minimum version of google-auth which is required to use the async_rest feature.

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Sep 20, 2024
@parthea parthea force-pushed the add-extra-for-aiohttp branch 7 times, most recently from 700a47f to 4e3842d Compare September 20, 2024 20:15
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Sep 20, 2024
@parthea parthea force-pushed the add-extra-for-aiohttp branch 3 times, most recently from 977a198 to c41a808 Compare September 20, 2024 20:23
@parthea parthea changed the title fix: add extra for aiohttp fix: add extra for async_rest Sep 20, 2024
@parthea parthea marked this pull request as ready for review September 20, 2024 20:27
@parthea parthea requested review from a team as code owners September 20, 2024 20:27
google/api_core/rest_streaming_async.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
testing/constraints-3.7.txt Show resolved Hide resolved
testing/constraints-async-rest-3.7.txt Show resolved Hide resolved
Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

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

One non-trivial comment about using the extras in setup.py

@ohmayr
Copy link
Contributor

ohmayr commented Sep 27, 2024

One non-trivial comment about using the extras in setup.py

What's the comment? I see that we're setting extras in setup.py here

@ohmayr ohmayr changed the title fix: add extra for async_rest fix: add async_rest extra to install async rest transport dependencies Sep 27, 2024
@ohmayr ohmayr changed the title fix: add async_rest extra to install async rest transport dependencies fix: add async_rest extra for async rest dependencies Sep 27, 2024
@ohmayr ohmayr changed the title fix: add async_rest extra for async rest dependencies chore: add async_rest extra for async rest dependencies Sep 27, 2024
noxfile.py Show resolved Hide resolved
@@ -110,7 +111,7 @@ def install_prerelease_dependencies(session, constraints_path):
session.install(*other_deps)


def default(session, install_grpc=True, prerelease=False, install_auth_aio=False):
def default(session, install_grpc=True, prerelease=False, install_async_rest=False):
Copy link
Contributor

@vchudnov-g vchudnov-g Sep 30, 2024

Choose a reason for hiding this comment

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

I suggest restructuring just slightly for brevity (I marked with ## the sections I changed). This depends on pip install foo[] with an empty extras list working, which I think it does. If not, one more line will be needed.

    install_extras = []    ##
    if install_grpc:       ##
        install_extras.append("grpc")  

    constraints_dir = str(CURRENT_DIRECTORY / "testing")
    if install_async_rest:   ##
        install_extras.append("async_rest")
        constraints_type = "async-rest-"
    else:
        constraints_type = ""

    if prerelease:
        install_prerelease_dependencies(
            session,
            f"{constraints_dir}/constraints-{constraints_type}{PYTHON_VERSIONS[0]}.txt",
        )
        # This *must* be the last install command to get the package from source.
        session.install(
            "-e", f".[{','.join(install_extras)}]", "--no-deps" ## see my other comment about installing grpc
        )
    else:
        constraints_file = (
            f"{constraints_dir}/constraints-{constraints_type}-{session.python}.txt"
        )
        # fall back to standard constraints file
        if not pathlib.Path(constraints_file).exists():
            constraints_file = f"{constraints_dir}/constraints-{session.python}.txt"

        session.install(
            "-e",
            f".[{','.join(install_extras)}]",  ##
            "-c",
            constraints_file,
        )

Copy link
Contributor

Choose a reason for hiding this comment

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

addressed.

noxfile.py Outdated
".[grpc]" if install_grpc else ".",
"-c",
f"{constraints_dir}/constraints-{session.python}.txt",
"-e", "." + ("[async_rest]" if install_async_rest else ""), "--no-deps"
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a pre-existing issue: at the top of the function we require that install_grpc be set if prerelease is set. But here we don't install grpc. Is that an oversight? It seems to me we probably should install it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

addressed.

)
# fall back to standard constraints file
if not pathlib.Path(constraints_file).exists():
constraints_file = f"{constraints_dir}/constraints-{session.python}.txt"
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a reminder at the top of the else that the constraints file does not determine whether the listed imports are imported.

Copy link
Contributor

Choose a reason for hiding this comment

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

addressed.

@ohmayr ohmayr merged commit 84bf637 into main Sep 30, 2024
37 checks passed
@ohmayr ohmayr deleted the add-extra-for-aiohttp branch September 30, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants