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

Downgrade spurious 'error' logs #1119

Merged
merged 2 commits into from
Nov 27, 2024
Merged

Conversation

ctcjab
Copy link
Contributor

@ctcjab ctcjab commented Nov 25, 2024

Since it is expected that not all embeddings model providers will be loadable, it is wrong to log the case that one cannot be loaded with level 'error'.

Resolves #839.

@krassowski krassowski added the bug Something isn't working label Nov 25, 2024
@krassowski
Copy link
Member

Thank you for seconding my case! I think either info or warning is fine here, but I think I would lean towards warning and not showing this message at all when the provider is not allowed.

@srdas
Copy link
Collaborator

srdas commented Nov 25, 2024

Thank you for seconding my case! I think either info or warning is fine here, but I think I would lean towards warning and not showing this message at all when the provider is not allowed.

+1 to @krassowski , all changes can be made in get_em_providers() and I think this can be approved. (1) Change .info to .warning and (2) check if provider is not in blocked_providers before throwing the warning.

Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@ctcjab Thanks for opening a PR to fix this! Just one small comment below.

@krassowski @ctcjab Does there exist authoritative documentation on whether info or warning should be used in these logs? I don't have much experience with producing/consuming logs, so you two are the experts here.

packages/jupyter-ai-magics/jupyter_ai_magics/utils.py Outdated Show resolved Hide resolved
@krassowski
Copy link
Member

(2) check if provider is not in blocked_providers before throwing the warning.

Let's address this separately as this is a more complex topic, I opened #1120 to track this

Since it is expected that not all model providers will be loadable, it
is wrong to log the case that one cannot be loaded with level 'error'.

Resolves jupyterlab#839.
@ctcjab ctcjab force-pushed the fix-spurious-error-logs branch from 53251f6 to 58c46a1 Compare November 26, 2024 22:55
@ctcjab ctcjab changed the title Change spurious 'error' log level to 'info' Downgrade spurious 'error' logs Nov 26, 2024
Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@ctcjab Great work, thank you!

@dlqqq dlqqq merged commit f8dc779 into jupyterlab:main Nov 27, 2024
12 checks passed
@dlqqq
Copy link
Member

dlqqq commented Nov 27, 2024

@meeseeksdev please backport to v3-dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error log when starting up extension
4 participants