iterative / dvclive

📈 Log and track ML metrics, parameters, models with Git and/or DVC
https://dvc.org/doc/dvclive
Apache License 2.0
161 stars 35 forks source link

add typehints to Live's public methods #770

Closed AlexandreKempf closed 7 months ago

AlexandreKempf commented 7 months ago

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

AlexandreKempf commented 7 months ago

@dberenbaum Thanks for the feedback. I'll take your suggestions into account. Meanwhile, I have a broader question on type hints. I agree that the current implementation for StrPath or ParamLike is cleaner the current way. But when an editor displays the help for each function, it is not very informative at first sight to have a StrPath type as a user, and I would rather have a str | pathlib.Path. Do you know a way to have the best of both worlds?

dberenbaum commented 7 months ago

Do you know a way to have the best of both worlds?

🤔 Good point. I don't know a way, but might be worth a little research. If you think it's helpful to have the more explicit types, I'm fine to keep them that way.

AlexandreKempf commented 7 months ago

I asked the question to tech people; we'll see.

I believe it will take more effort to maintain, but it will be way more user-friendly to have a list of possible plots like ["linear," "simple, ...] for the user to play with and discover the tool. Rather than an opaque [*BASE_SCHEMA.schema["type"].validators]. (I took this one from the studio-event data format).

You see my point, I guess, we just need to figure out own much a pain it will be to maintain to know if it is worth it :)

dberenbaum commented 7 months ago

As mentioned by @jonburdo, I take back importing from elsewhere for type checking. It seems like an anti-pattern with more thought, and we are better off listing the choices or probably even just leaving the type hints more general for now.

AlexandreKempf commented 7 months ago

I agree with you two after the explanation from @jonburdo. I still believe the user-facing functions should have a clear and readable type from the editor. But it is not an urgent matter, and we can always get back at it later.

dberenbaum commented 7 months ago

@AlexandreKempf Could you check the CI failures?

jonburdo commented 7 months ago

I believe it will take more effort to maintain, but it will be way more user-friendly to have a list of possible plots like ["linear," "simple, ...] for the user to play with and discover the tool.

I think you're right for a relatively short list like this. I sometimes take a look at other popular libraries for questions like this. Running rg '\bLiteral\[' (ripgrep) in the numpy and typeshed repos shows they generally just repeat the whole thing rather than using a variable. I imagine it won't be too hard to maintain.

AlexandreKempf commented 7 months ago

Thanks for the advice @jonburdo. What do you think about that @dberenbaum? I would say we go with Jon's advice here: our lists are small and useful for users for editors' suggestions. If it is too hard to maintain, we can always revert it at no cost :)

dberenbaum commented 7 months ago

Yes, I don't think it's worth worrying about the maintenance costs at this point