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

Move kwargs from compute to a config pass during load #169

Closed
lvwerra opened this issue Jun 29, 2022 · 3 comments · Fixed by #188
Closed

Move kwargs from compute to a config pass during load #169

lvwerra opened this issue Jun 29, 2022 · 3 comments · Fixed by #188

Comments

@lvwerra
Copy link
Member

lvwerra commented Jun 29, 2022

Currently, there is a mix between configs that are used when the metric is instantiated and configs that are later passed to compute. This makes life harder in several ways as pointed out in #137 and #138 and also makes the combine function from #150 harder to use. The two main points:

  • It is hard to know in advance what configs are available (e.g. useful for eval on the hub)
  • It is hard to configure metrics in advance that are used in a wrapped object. One always needs to pass the kwargs downstream (affects evaluator and combine).

I think we could solve this by moving all kwargs to the load. If a user wants to run a metric with different configs they can just load several instances of the metric which is cheap. In addition we could wrap the configs in something like a dataclass that specifies the types of configs and options when only a limited number of options are available (e.g. F1 can only be use binary or multilabel).

What do you think? @lewtun @lhoestq @sashavor

@lewtun
Copy link
Member

lewtun commented Jun 30, 2022

I like this proposal to move all kwargs to the load() function a lot!

To help gauge the difference, could you share some pseudocode that demonstrates what the new API would look like for a "simple" (few kwargs) vs "complex" metric (mix of configs and kwargs)?

@hfawaz
Copy link

hfawaz commented Oct 30, 2022

Is this actually solved since it is reverted in #299 ?

@christian-storm
Copy link

I'm also confused as to where this is at? I'm not even sure if this is even where it is being tracked.

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.

4 participants