add option to load metrics with kwargs

[jpcbertoldo]

Description

• Fixes impossible to pass args to the metrics #687

Changes

• Bug fix (non-breaking change which fixes an issue)
• Refactor (non-breaking change which refactors the code base)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist

• My code follows the pre-commit style and check guidelines of this project.
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing tests pass locally with my changes

[samet-akcay]

I agree that it would be good to follow the same convention as the PL CLI.
Only few minor comments.

[jpcbertoldo]

Since i'm working on this file, could be nice to solve #685 as well?

[samet-akcay]

Since i'm working on this file, could be nice to solve #685 as well?

Apologies, just seen that issue. Yeah, I agree. It would make sense to move it to test helpers if the tests are the only place of use.

[jpcbertoldo]

Both done

[jpcbertoldo]

I adressed the codacy checks above.

[jpcbertoldo]

Should this go to docs or can we just assume people will guess it because it's consistent with other parts of the config? 🙃

[samet-akcay]

Should this go to docs or can we just assume people will guess it because it's consistent with other parts of the config? 🙃

would be good to document it somewhere; otherwise, it might stay as a hidden gem :)

Not sure about where to document it though. @ashwinvaidya17, any thoughts?

[ashwinvaidya17]

Thanks for the efforts. This is a good addition to metrics configuration flexibility. I have a few comments though.

[ashwinvaidya17]

Should this go to docs or can we just assume people will guess it because it's consistent with other parts of the config? 🙃

would be good to document it somewhere; otherwise, it might stay as a hidden gem :)

Not sure about where to document it though. @ashwinvaidya17, any thoughts?

I agree if we now support this it would be nice to mention it somewhere as well. How about docs/source/reference_guide/api/metrics.rst? We can have it above automodule.

[jpcbertoldo]

I believe i addressed everything.

[samet-akcay]

I believe i addressed everything.

Thanks @jpcbertoldo. Can you please check the failed tests?

[jpcbertoldo]

I believe i addressed everything.

Thanks @jpcbertoldo. Can you please check the failed tests?

Done.

@ashwinvaidya17 I forgot to mention that there was a check bothering about the fact that a few dummy classes where repeated in the tests so i put them in tests/helpers/dummy.py

[samet-akcay]

I believe i addressed everything.

Thanks @jpcbertoldo. Can you please check the failed tests?

Done.

@ashwinvaidya17 I forgot to mention that there was a check bothering about the fact that a few dummy classes where repeated in the tests so i put them in tests/helpers/dummy.py

@jpcbertoldo, thanks for organising them.
The tests needs another round of refactoring anyway, which we were planning on executing sometime this quarter. Would be a start.

[samet-akcay]

I believe i addressed everything.

Thanks @jpcbertoldo. Can you please check the failed tests?

Done.

@jpcbertoldo, looks like the tests still fail unfortunately :/

[jpcbertoldo]

I dont understand, it was all green last night 🤨🤔. Ok will take a look.

[ashwinvaidya17]

I believe i addressed everything.

Thanks @jpcbertoldo. Can you please check the failed tests?

Done.

@ashwinvaidya17 I forgot to mention that there was a check bothering about the fact that a few dummy classes where repeated in the tests so i put them in tests/helpers/dummy.py

Thanks for doing that. We have a lot of dummy modules and dataloaders peppered around the tests. That's something that needs to be addressed in the redesign as well.

[ashwinvaidya17]

Thanks. One minor comment.

[jpcbertoldo]

I believe i addressed everything.

Thanks @jpcbertoldo. Can you please check the failed tests?

Done.

@jpcbertoldo, looks like the tests still fail unfortunately :/

Ok, it was the test that had a little refactor, my bad!

=========================== short test summary info ============================
ERROR tests/pre_merge/utils/callbacks/visualizer_callback/test_visualizer.py
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!

I think it'lll fix.
I'm struggling to run tox locally because of some env var so let's see...

[ashwinvaidya17]

Thanks

[samet-akcay]

@jpcbertoldo, thanks looking good now!