dspy icon indicating copy to clipboard operation
dspy copied to clipboard

Add docstrings and type hints to Evaluate class.

Open rpgoldman opened this issue 1 year ago • 6 comments

Add docstrings and type hints for the Evaluate class.

Needs review!

rpgoldman avatar Apr 30 '24 00:04 rpgoldman

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.

rpgoldman avatar Apr 30 '24 14:04 rpgoldman

With help from the mypy folks, the type errors are now fixed, and mypy likes the file.

rpgoldman avatar May 02 '24 16:05 rpgoldman

@arnavsinghvi11 @rpgoldman what is left for this to be merged?

cdowellmdb avatar Jun 11 '24 21:06 cdowellmdb

@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?

rpgoldman avatar Jun 13 '24 19:06 rpgoldman

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.

arnavsinghvi11 avatar Jun 15 '24 18:06 arnavsinghvi11

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:

  1. We add a method like increment_assert_failures to the sort of thing that counts assertion and suggestion failures and use that instead.
  2. 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.

rpgoldman avatar Jun 17 '24 14:06 rpgoldman