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

Verbose setting overwrite custom logger level. #123

Closed
Carreau opened this issue Jul 21, 2024 · 3 comments · May be fixed by #124
Closed

Verbose setting overwrite custom logger level. #123

Carreau opened this issue Jul 21, 2024 · 3 comments · May be fixed by #124

Comments

@Carreau
Copy link
Contributor

Carreau commented Jul 21, 2024

Transcations also have a verbose attribute which is unused.

def install(..., verbose=<value default False>) overwrite a custom logger level. This and a few other places should allow None meaning "don't change the level".

@Carreau
Copy link
Contributor Author

Carreau commented Jul 21, 2024

(in my case it is overwritting to a lower level as I start my session with a getLogger('micropip').setLevel(DEBUG).

Carreau added a commit to Carreau/micropip that referenced this issue Jul 21, 2024
This improve the handling of indesx and error messages.

Before this PR all errors reaching an index woudl be ValueError, thus
not findig a wheel was the same as the index page having a non-handled
content type, or even failing to parse the html because of a typo.

This made it hard to debug, or even realise that an index url was wrong,
or that the pages served by the index were incorrect.

I thus introduce 3 New Errors that do not inherit from `ValueError`:
 - IndexMetadataFetchError
 - UnsupportedParserContentTypeError
 - WheelNotFoundError

The first two of which trigger real error, as they likely suggest a
problem with the index or the user config, and should not be ignored.

In addition the `verbose=` option was unconditionally setting the log
level. I think this is imporper as if the logging level is set somewhere
else (to DEBUG, or something else), then it is overwritten.

- This then adds the option to pass `verbose=None`, (and make it the
  default), in which case it will not change the default.

python -m http.server will serve ContentType with `; charset=8tf-8`,
which is not recognized.

- This now handle the case where the index ContentType header
  contains parameters for example `; chartype=utf-8`, by discarding
  everything after the semicolon `;`; this is not proper parsing, but
  should be sufficient here.

 - I found that more debugg logging woudl be useful and added a number
   of debug logs calls

With this I appread to get proper error message and debug statements
when trying to work on the Cors proxy.

Should close pyodide#123, and pyodide#121, and help with pyodide/pyodide#4898
Carreau added a commit to Carreau/micropip that referenced this issue Jul 21, 2024
This improve the handling of indexes and error messages.

Before this PR all errors reaching an index would be ValueError, thus
not finding a wheel was the same as the index page having a non-handled
content type, or even failing to parse the html because of a typo.

This made it hard to debug, or even realize that an index url was wrong,
or that the pages served by the index were incorrect.

I thus introduce 3 New Errors that do not inherit from `ValueError`:
 - IndexMetadataFetchError
 - UnsupportedParserContentTypeError
 - WheelNotFoundError

The first two of which trigger real error, as they likely suggest a
problem with the index or the user config, and should not be ignored.

In addition the `verbose=` option was unconditionally setting the log
level. I think this is improper as if the logging level is set somewhere
else (to DEBUG, or something else), then it is overwritten.

- This then adds the option to pass `verbose=None`, (and make it the
  default), in which case it will not change the default.

python -m http.server will serve ContentType with `; charset=utf-8`,
which is not recognized.

- This now handle the case where the index ContentType header
  contains parameters for example `; charset=utf-8`, by discarding
  everything after the semicolon `;`; this is not proper parsing, but
  should be sufficient here.

 - I found that more debug logging would be useful and added a number
   of debug logs calls

With this I appear to get proper error message and debug statements
when trying to work on the Cors proxy.

Should close pyodide#123, and pyodide#121, and help with pyodide/pyodide#4898
@ryanking13
Copy link
Member

Thanks, that sounds reasonable to me.

I wonder if it would be better to remove the verbose parameter at all, and let users externally set getLogger('micropip').setLevel() to chang the log level. I introduced the verbose parameter to help users see what micropip does internally, but probably it is a bad practice logging-wise.

@Carreau Carreau closed this as completed Sep 19, 2024
@Carreau
Copy link
Contributor Author

Carreau commented Sep 19, 2024

Should have been fixed by #132

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 a pull request may close this issue.

2 participants