-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
FEAT allow configuring automatically requested metadata #31401
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
base: main
Are you sure you want to change the base?
Conversation
❌ Linting issuesThis PR is introducing linting issues. Here's a summary of the issues. Note that you can avoid having linting issues by enabling You can see the details of the linting issues under the
|
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 like the naming of "auto-requests", that I think is very intuitive.
Still, auto-requests here are contrasted with "empty" metadata requests and "default" metadata requests. If I understood correctly, then the "default" is "empty" requests, so these should both be referred to via the same terms.
Auto-requested metadata. These metadata request values override default | ||
request values if `set_config(metadata_request_policy="auto")` is set. |
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.
Maybe explain this a bit better:
Auto-requested metadata. These metadata request values override default | |
request values if `set_config(metadata_request_policy="auto")` is set. | |
Auto-requested metadata. Keyword arguments where the key is a method | |
name (e.g., 'fit', 'predict') and the value is a metadata request | |
(either a str or a list of str). These override default request values | |
if `set_config(metadata_request_policy="auto")` is set. |
# Here's an example demonstrating both approaches: | ||
|
||
|
||
class DefaultRoutingClassifier(ClassifierMixin, BaseEstimator): |
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.
Could we rename this, since it's not a router?
class DefaultRoutingClassifier(ClassifierMixin, BaseEstimator): | |
class DefaultClassifier(ClassifierMixin, BaseEstimator): |
- `"auto"`: Metadata is requested if the consumer has flagged it as an | ||
auto-request. |
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.
Trying to find a formulation that communicates that this entails that a sub-part of the metadata can be requested. I think it's not clear otherwise.
I would also avoid talking about "the" consumer, since there can be several.
- `"auto"`: Metadata is requested if the consumer has flagged it as an | |
auto-request. | |
- `"auto"`: Only the subset of metadata flagged by consumers for | |
auto-request will be requested. |
Parameters | ||
---------- | ||
**auto_requests : str or list of str | ||
Auto-requested metadata. These metadata request values override default |
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 default are the empty requests, right?
Auto-requested metadata. These metadata request values override default | |
Auto-requested metadata. These metadata request values override empty |
Alternative to #30946
The approach here is to avoid adding a new function to the base class, and instead handle it via the same
get_metadata_routing
.However, this solution changes the signature of the method, which is not idea. We could at the same time deprecate the method and replace it with a
__sklearn_get_metadata_routing__
method with a new signature, and the__sklearn__
pattern as the rest of the developer API we have now.cc @antoinebaker @ogrisel @StefanieSenger
This is not complete, putting here for us to be able to have a more concrete view of the idea.
The idea is that developer / user code looks like this: