dspy
dspy copied to clipboard
Add docstrings and type hints to Evaluate class.
Add docstrings and type hints for the Evaluate class.
Needs review!
Still some typing errors:
dspy/evaluate/evaluate.py:17: error: Cannot find implementation or library stub for module named "IPython.display" [import-not-found]
dspy/evaluate/evaluate.py:147: error: Overloaded function signatures 1 and 2 overlap with incompatible return types [overload-overlap]
dspy/evaluate/evaluate.py:147: error: Overloaded function signatures 1 and 3 overlap with incompatible return types [overload-overlap]
dspy/evaluate/evaluate.py:161: error: Overloaded function signatures 2 and 3 overlap with incompatible return types [overload-overlap]
dspy/evaluate/evaluate.py:264: error: "type[tqdm[Any]]" has no attribute "_instances" [attr-defined]
dspy/evaluate/evaluate.py:303: error: "Series[Any]" not callable [operator]
Will check these as type permits.
With help from the mypy folks, the type errors are now fixed, and mypy likes the file.
@arnavsinghvi11 @rpgoldman what is left for this to be merged?
@cdowellmdb I think it's good to go. @arnavsinghvi11 didn't like the comments I put in to muffle linters; I prefer to keep them. Is that a show-stopper?
If not, do you need me to merge in the changes since this was pushed?
Thanks for circling back to this @rpgoldman @cdowellmdb ! Left a few comments back on the review and it should be good to merge once the last parts are resolved! (There's also a merge conflict but I believe rebasing to main should fix this).
The in-line comments are definitely not a show-stopper, but I'd prefer to leave them out from committed code to ensure they catch any changes needed.
The in-line comments are definitely not a show-stopper, but I'd prefer to leave them out from committed code to ensure they catch any changes needed.
Not sure I understand this last. What's "they" here? I assume you are saying that we should continue to see these linter warnings. Here's an example of why I disagree:
E.g., I put in # noinspection PyProtectedMember because that piece of code uses a private member:
program._assert_failures += dspy.settings.get("assert_failures")
One of 2 things should happen, IMO:
- We add a method like
increment_assert_failuresto the sort of thing that counts assertion and suggestion failures and use that instead. - We decide we want to continue to use the protected member, and as I do, quash the warning.
I'm ok with either solution, but a third solution, where we just have linters continue shout at us, seems generally bad. Bad because if we end up with lots of these linter shoutings then it's just human nature to start ignoring the linter warnings altogether. And we certainly cannot put "must build cleanly" into our testing process, if we do this.
I have been bitten many times by code repositories that emit lots of warnings, training their developers to ignore linter warnings. Because sooner or later there's a warning that's really important, but that signal is lost in the noise. I'm a strong believer in building clean. If there's a warning, either it's real and you fix it, or it's not real and you quash it.