-
Notifications
You must be signed in to change notification settings - Fork 146
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
feat: deprecate use_thread #949
base: 12-23-feat_change_mutation_detection_and_allow_reactive_boolean_defaults
Are you sure you want to change the base?
feat: deprecate use_thread #949
Conversation
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I think in this PR we should also export use_task/task from the solara namespace, and maybe also have wrapper functions in solara.lab.use_task to warn that it's not in lab anymore?
Also, I think the default argument for dependencies for use_task should be None, so that it does not execute by default (this make the default use_task and task more similar)
solara/util.py
Outdated
def decorator(func): | ||
@functools.wraps(func) | ||
def wrapped(*args, **kwargs): | ||
warnings.simplefilter("always", DeprecationWarning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should call this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason this was originally included is that Python doesn't show DeprecationWarning
s by default. However, it seems that it is recommended to use FutureWarning
for warnings that you want users to see (as per scikit learn), so I made the warning category configurable by default instead.
5e49cc6
to
1c1a375
Compare
3cdb9d4
to
03b2dcd
Compare
1c1a375
to
f16af91
Compare
202ce98
to
5b8ba9c
Compare
f16af91
to
6bfa775
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, the only issue is that we want to have a redirect for old links.
We already do that in |
5b8ba9c
to
804d657
Compare
6bfa775
to
f776b7c
Compare
804d657
to
c457ccb
Compare
f776b7c
to
d77b641
Compare
d77b641
to
6582fbf
Compare
All Submissions:
pre-commit
prior to committing my changes (see development setup docs).Changes to / New Features:
Description of changes
Deprecate
use_thread
in favour ofuse_task
andTask
.Closes #784