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

More debug logging #142

Merged
merged 1 commit into from
Oct 18, 2024
Merged

More debug logging #142

merged 1 commit into from
Oct 18, 2024

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Oct 18, 2024

Basically still trying to make the proxy works, but so many errors are swallowed because everything that triggers a ValueError is just ignore. So add a bunch of debug to at least be able to debug.

async def add_requirement_inner(
    self,
    req: Requirement,
) -> None:
    ....
    try:
        if self.search_pyodide_lock_first:
            ...
        else:
            try:
                await self._add_requirement_from_package_index(req)
            except ValueError:
                # basically this swallow things it should not, like
                improper content type and likely a bunch of other.

This is where many ValueError are swallowed, I will send more changes to
change various error types.

@hoodmane
Copy link
Member

hoodmane commented Oct 18, 2024

Logging changes look good.

We seem to raise a LOT of ValueErrors. The comment in transaction.py says: "If the requirement is not found in package index" which means that it's expecting to catch not found packages. Maybe make:

class NotFoundError(Exception):
    pass

class NoSuchPackage(NotFoundError):
    pass

class NoCompatibleWheel(NotFoundError):
   pass

and then raise these in querty_package and find_wheel and switch the two except ValueError spots to catch NotFoundError?

@Carreau
Copy link
Contributor Author

Carreau commented Oct 18, 2024

I'm not 100% sure about the hierarchy, the problem is (to me) that NotFoundError, strongly screams 404 error, and AFAICT in your example NoSuchPackage and NotFoundError would then be the same.

And NoCompatibleWheel is really not a subclass of NotFound.

Maybe I'll split the logging into it's own things and then refactor the various errors.

Basically still trying to make the proxy works, but so many errors are
swallowed because everything that triggers a ValueError is just ignore.
So add a bunch of debug to at least be able to debug.

    async def add_requirement_inner(
        self,
        req: Requirement,
    ) -> None:
        ....
        try:
            if self.search_pyodide_lock_first:
                ...
            else:
                try:
                    await self._add_requirement_from_package_index(req)
                except ValueError:
                    # basically this swallow things it should not, like
                    improper content type and likely a bunch of other.

This is where many ValueError are swallowed, I will send more changes to
change various error types.
@Carreau Carreau changed the title More logging, and differentiate ValueErrors More debug logging Oct 18, 2024
@Carreau
Copy link
Contributor Author

Carreau commented Oct 18, 2024

I removed the change of exceptions and will send a subsequent PR.

case (
"application/vnd.pypi.simple.v1+html"
| "text/html"
| "text/html; charset=utf-8"
Copy link
Member

Choose a reason for hiding this comment

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

I guess there is also this change, but it seems minor enough and correct enough to merge without worrying too much.

@hoodmane hoodmane merged commit 4044983 into pyodide:main Oct 18, 2024
4 checks passed
@hoodmane
Copy link
Member

Thanks!

@hoodmane
Copy link
Member

And NoCompatibleWheel is really not a subclass of NotFound.

I guess in that case the two catch blocks can be:

except WheelNotFound, NoCompatibleWheelFound:

or something?

Carreau added a commit to Carreau/micropip that referenced this pull request Oct 18, 2024
ValueErrors are a bit too generic, and by raising/catching them we loose
a potential useful information; For example an invalid content type for
the index, is dealt as the same way than a missing wheel.

This introduces new exceptions for various cases, ant stop catching the
now UnsupportedContentTypeError, as I believe it should be a hard error as
we are likely having either an error with the index software, or a user
configuraition error.

See also beginning of discussion in pyodide#142
@Carreau
Copy link
Contributor Author

Carreau commented Oct 18, 2024

Yep, except on Python 3 you need parentheses :-)

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.

2 participants