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

allowed_providers/blocked_providers no longer work if only one is given #913

Open
krassowski opened this issue Jul 26, 2024 · 6 comments
Open
Labels
bug Something isn't working

Comments

@krassowski
Copy link
Member

krassowski commented Jul 26, 2024

Description

allowed_providers/blocked_providers no longer work if only one is given. This is a pretty severe regression for administrators of tightly controlled deployments.

Reproduce

with traitlets 5.13.0 and 5.14.3:

jupyter lab --AiExtension.allowed_providers=X
[ AiExtension] Configured provider allowlist: None

with traitlets 5.12.0

jupyter lab --AiExtension.allowed_providers=X
[ AiExtension] Configured provider allowlist: ['X']

With all versions of traitlets:

jupyter lab --AiExtension.allowed_providers=X --AiExtension.allowed_providers=Y
[ AiExtension] Configured provider allowlist: ['X', 'Y']

Expected behavior

When adding this feature I added tests which would have caught it (#415). Subsequently these were disabled (#446).

I would appreciate if these tests could be restored.

Context

Extension 2.19.1 and earlier.

@dlqqq
Copy link
Member

dlqqq commented Jul 27, 2024

Thank you for reporting this issue! I certainly did make a mistake by not following up and re-enabling those tests.

We plan to perform a release early next week (within 3-4 days), so I'll see if I can come up with a patch by then. Feel free to work on this issue in parallel as well if you'd like.

@krassowski
Copy link
Member Author

The bug is in ipython/traitlets#908. It would be trivial to revert the problematic change but might be hard to solve cleanly. I spent a lot of time debugging this and might not have time to work on a fix anytime soon.

In any case the only thing to do in juypter-ai is to increase test coverage for CLI options and re-enable skipped tests.

@krassowski
Copy link
Member Author

This also affects other List traits with allow_none=True, namely --AiExtension.allowed_models and --AiExtension.blocked_models. I tested that Dict traits work fine on --AiExtension.model_parameters.

@dlqqq
Copy link
Member

dlqqq commented Jul 29, 2024

traitlets==5.13.0 was released on PyPI on October 30, 2023. Since this has been an issue for a very long time, I don't think it would be wise to include a hasty patch for this issue in the upcoming release today. Adding a version ceiling like <5.13.0 could have unintended side effects. I'll focus on fixes/features for the next release, then work on this later this week. Feel free to ping me if you make any findings.

@krassowski
Copy link
Member Author

I think that maybe we can fix it in jupyter-ai by specifying allow_none = False and adding a default=[]. I will try it another day. I agree no need to rush here as affected downstreams can pin trailets locally.

@krassowski
Copy link
Member Author

traitlets==5.13.0 was released on PyPI on October 30, 2023

Yes, one week after #415 was merged. And on November 8th the test was disabled in #446, which I suspect was liekly because it was not passing locally because someone may have had the new version of traitlets installed locally but CI was still using the older/cached version.

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

No branches or pull requests

2 participants