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

BUG: fetch_string_and_headers compat: raise in and out of pyodide #129

Merged
merged 4 commits into from
Oct 5, 2024

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Aug 6, 2024

Currently only the not_in_pyodide will raise on non-success, because this is the default behavior of urllib, the in_pyodide will not, so I added a raise_for_status.

It is better to raise, as otherwise the package parser will potentially get proper URL and not manage to parse it, and decide there is no wheels, while we actually just got an error (404, or maybe 500).

In addition wraps both case in a custom local HttpStatusError, so that we can actually catch these errors in the right places when we encounter them.

I think we could define HttpStatusError on the ABC and not make it an ABC itself, but it small enough that I think duplication is ok.

from micropip._compat import HttpStatusError, fetch_string_and_headers


@pytest.mark.asyncio
Copy link
Contributor Author

@Carreau Carreau Aug 6, 2024

Choose a reason for hiding this comment

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

Do I just need to throw a @run_in_pyodide here to make sure this is tested in and out of pyodide ? Or is that a bit more complicated ?

Copy link
Member

Choose a reason for hiding this comment

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

It's a bit more complicated. Honestly, the easiest thing might be just to copy paste the code. At some point we'll have to think about a better way...

Copy link
Member

@ryanking13 ryanking13 Aug 7, 2024

Choose a reason for hiding this comment

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

Yep, testing something both in and out of Pyodide is a bit complex. Actually, I think you don't need to test out-of-pyodide case. We don't care about using micropip out of Pyodide and not_in_pyodide exists mostly for test purpose and typing.

@Carreau
Copy link
Contributor Author

Carreau commented Aug 12, 2024

Moving to draft, I think HttpStatusError is not yet in latest released pyodide.

@Carreau Carreau marked this pull request as draft August 12, 2024 09:31
@Carreau Carreau changed the title BUG: fetch_string_and_headers compat: raise in and out of pyodide [Need newer pyodide] BUG: fetch_string_and_headers compat: raise in and out of pyodide Aug 12, 2024
@Carreau Carreau marked this pull request as ready for review August 21, 2024 09:48
@Carreau
Copy link
Contributor Author

Carreau commented Aug 21, 2024

Ok, I've added relevant fixes now that PyPI properly set CORS on 404s, and added conditional for wether we are on pyodide 0.26.x or 0.27+.

@Carreau Carreau changed the title [Need newer pyodide] BUG: fetch_string_and_headers compat: raise in and out of pyodide BUG: fetch_string_and_headers compat: raise in and out of pyodide Aug 21, 2024
@Carreau
Copy link
Contributor Author

Carreau commented Aug 21, 2024

SO now some of the fixture that return 500 now need to return 404... I'm not sure how to get the httpserver fixture to return 404 on wildcard url without expecting requests.

@ryanking13
Copy link
Member

ValueError: Function test_404_raise's first argument name 'httpserver' should start with 'selenium'

@run_in_pyodide should be the inner-most decorator.

tests/test_compat.py Outdated Show resolved Hide resolved
@ryanking13
Copy link
Member

Let's release Pyodide 0.27.0a1 and use it in the micropip CI so we don't need to put complex version checks. Hood released 0.27.0a1 yesterday but the deploy was failed because of the CI timeout (pyodide/pyodide@5231ac0). Let me see if rerunning the job would work.

@Carreau
Copy link
Contributor Author

Carreau commented Aug 21, 2024

Let's release Pyodide 0.27.0a1 and use it in the micropip CI

I was also just concern micropip might be broken with current pyodide/warehouse, but a new version of pyodide would be great :-)

@Carreau
Copy link
Contributor Author

Carreau commented Aug 21, 2024

Ok, I've tried multiple variation of the test to @run_in_pyodide, but can't figure out how to actually have one that works.



@run_in_pyodide
async def _inner_test_404_raise(httpserver):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async def _inner_test_404_raise(httpserver):
async def _inner_test_404_raise(selenium, httpserver):

Copy link
Member

Choose a reason for hiding this comment

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

Also the httpserver fixture isn't going to work with @run_in_pyodide. I'm not sure what the best way is to do this, @ryanking13 do you have any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

You can do this way (passing only string not httpserver fixture)

def test_404(httpserver):
    @run_in_pyodide
    async def _inner_test_404_raise(url):
        from ... import fetch_string_and_headers
        import pytest
        with pytest.raises(HttpStatusError):
            await fetch_string_and_headers(url, {})

    httpserver.expect_request("/404").respond_with_data(
        "Not found", status=404, content_type="text/plain"
    )
    url_404 = httpserver.url_for("/404")
    _inner_test_404_raise(url_404)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... let me try that, thanks for the pointers.

@hoodmane
Copy link
Member

I released 0.27.0a2 instead.

@Carreau Carreau force-pushed the status-raises branch 5 times, most recently from a502d5e to 6d73dd2 Compare August 23, 2024 08:38
@Carreau
Copy link
Contributor Author

Carreau commented Aug 23, 2024

I released 0.27.0a2 instead.

BTW should CI be updated

- pyodide-version: [0.24.1, 0.25.0]
+ pyodide-version: [0.24.1, 0.25.0, 0.26.0, 0.27.0a2]

?

@Carreau
Copy link
Contributor Author

Carreau commented Sep 16, 2024

Any idea on how to actually change the timeout to something else the 10 minutes... I've tried various options with no success.

@ryanking13
Copy link
Member

Any idea on how to actually change the timeout to something else the 10 minutes... I've tried various options with no success.

I think this is a chrome (+selenium) issue.. for now I just pinned the version of the chrome to some older version to see if it pass. #135

@Carreau
Copy link
Contributor Author

Carreau commented Sep 17, 2024

I think this is a chrome (+selenium) issue.. for now I just pinned the version of the chrome to some older version to see if it pass. #135

More generally when I write a test with an error the timeout is also 10 min, and I could not find how to make it timeout in less... but thanks for #135

tests/conftest.py Outdated Show resolved Hide resolved
@Carreau Carreau force-pushed the status-raises branch 5 times, most recently from 1523d5a to 11f61d2 Compare September 19, 2024 12:09
Currently only the not_in_pyodide will raise on non-success, because this
is the default behavior of urllib, the in_pyodide will not, so I added a
raise_for_status.

It is better to raise, as otherwise the package parser will potentially
get proper URL and not manage to parse it, and decide there is no
wheels, while we actually just got an error (404, or maybe 500).

In addition wraps both case in a custom local HttpStatusError, so that
we can actually catch these errors in the right places when we encounter
them.

Also add handling for PyPI 404

Now that warehouse set cors to 404, (pypi/warehouse#16339)
we need to change the checked exceptions as there is no more network
errors.
Comment on lines 13 to 20
if parse(pyodide.__version__) > parse("0.27"):
from pyodide.http import HttpStatusError, pyfetch
else:

class HttpStatusError(Exception): # type: ignore [no-redef]
"""we just want this to be defined, it is never going to be raised"""

pass
Copy link
Member

Choose a reason for hiding this comment

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

I removed Pyodide versions < 0.27 from CI. I am fine with removing these conditions and drop backward compatibility for versions < 0.27 @hoodmane WDYT?

Comment on lines 289 to 299
try:
import pyodide
from packaging.version import parse

if parse(pyodide.__version__) > parse("0.27"):
# reraise on more recent pyodide.
raise
continue
except ImportError:
# not in pyodide.
raise
Copy link
Member

Choose a reason for hiding this comment

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

Here too.

@Carreau
Copy link
Contributor Author

Carreau commented Oct 1, 2024

ok, I pushed a commit that remove compat with old versions.

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks @Carreau!

@ryanking13 ryanking13 merged commit 5139bbd into pyodide:main Oct 5, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants