python-wheel-build / fromager

Build your own wheels
https://fromager.readthedocs.io/en/latest/
Apache License 2.0
5 stars 9 forks source link

Fromager 361: Requsts Session Object #372

Closed rd4398 closed 2 weeks ago

rd4398 commented 2 weeks ago

Fixes: https://github.com/python-wheel-build/fromager/issues/361

rd4398 commented 2 weeks ago

Instead of passing it in context, should we create a module instead that exposes this session object. Then instead of passing the context everywhere, we just import the session object?

I had tried separate module few weeks ago in https://github.com/python-wheel-build/fromager/pull/228 The suggestions made there is why I am doing this in context instead of separate module

shubhbapna commented 2 weeks ago

I had tried separate module few weeks ago in #228 The suggestions made there is why I am doing this in context instead of separate module

Oh is this the module: https://github.com/python-wheel-build/fromager/pull/228/commits/4cc20c85a5e30e21ac02f4eb3bc7771ff5faf0ba ? The module in this commit defines a class but there is no object that can be used directly on importing. I was thinking something along these lines:

request_session.py:

import request
session = request.session()

Then say in resolver.py:

from .request_session import session

# the function signature doesn't change at all
def get_project_from_pypi(
    project: str,
    extras: typing.Iterable[str],
    sdist_server_url: str,
) -> typing.Iterable[Candidate]:
    """Return candidates created from the project name and extras."""
    simple_index_url = sdist_server_url.rstrip("/") + "/" + project + "/"
    logger.debug("%s: getting available versions from %s", project, simple_index_url)
    # using the session object directly
    data = session.get(simple_index_url).content
rd4398 commented 2 weeks ago

I had tried separate module few weeks ago in #228 The suggestions made there is why I am doing this in context instead of separate module

Oh is this the module: 4cc20c8 ? The module in this commit defines a class but there is no object that can be used directly on importing. I was thinking something along these lines:

request_session.py:

import request
session = request.session()

Then say in resolver.py:

from .request_session import session

# the function signature doesn't change at all
def get_project_from_pypi(
    project: str,
    extras: typing.Iterable[str],
    sdist_server_url: str,
) -> typing.Iterable[Candidate]:
    """Return candidates created from the project name and extras."""
    simple_index_url = sdist_server_url.rstrip("/") + "/" + project + "/"
    logger.debug("%s: getting available versions from %s", project, simple_index_url)
    # using the session object directly
    data = session.get(simple_index_url).content

yeah, that's the one. Hmm okay, I can do that

rd4398 commented 2 weeks ago

I had tried separate module few weeks ago in #228 The suggestions made there is why I am doing this in context instead of separate module

Oh is this the module: 4cc20c8 ? The module in this commit defines a class but there is no object that can be used directly on importing. I was thinking something along these lines: request_session.py:

import request
session = request.session()

Then say in resolver.py:

from .request_session import session

# the function signature doesn't change at all
def get_project_from_pypi(
    project: str,
    extras: typing.Iterable[str],
    sdist_server_url: str,
) -> typing.Iterable[Candidate]:
    """Return candidates created from the project name and extras."""
    simple_index_url = sdist_server_url.rstrip("/") + "/" + project + "/"
    logger.debug("%s: getting available versions from %s", project, simple_index_url)
    # using the session object directly
    data = session.get(simple_index_url).content

yeah, that's the one. Hmm okay, I can do that

Done

dhellmann commented 2 weeks ago

I'm happy with this version. @shubhbapna had comments, so I'll leave it open for him to review.

I re-ran the coverage job that failed and it failed again. Is that a known problem?

rd4398 commented 2 weeks ago

I'm happy with this version. @shubhbapna had comments, so I'll leave it open for him to review.

I re-ran the coverage job that failed and it failed again. Is that a known problem?

I am not sure. I am seeing it for the first time