Add docstrings and type hints to Evaluate class. #935

rpgoldman · 2024-04-30T00:45:25Z

Add docstrings and type hints for the Evaluate class.

Needs review!

rpgoldman · 2024-04-30T14:47:39Z

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 kind help from the mypy folks, fixed the overload hints for the `__call__` method on Evaluate.

rpgoldman · 2024-05-02T16:45:54Z

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

arnavsinghvi11 · 2024-05-05T23:24:52Z

dspy/evaluate/evaluate.py


+# noinspection PyUnresolvedReferences


are these comments needed? prefer to remove non-code related comments

These are used to quash linter complaints. I'd prefer to keep them so that I don't see the same linter complaints over and over.

we don't maintain these comments throughout the repo and it's actually fine to flag the linter and add in the changes within the PR. would prefer to remove them.

arnavsinghvi11 · 2024-05-05T23:25:00Z

dspy/evaluate/evaluate.py


 try:
- from IPython.display import HTML
- from IPython.display import display as ipython_display
+ # noinspection PyPackageRequirements


here as well

arnavsinghvi11 · 2024-05-05T23:25:19Z

dspy/evaluate/evaluate.py

 except ImportError:
 ipython_display = print
-
- def HTML(x):
+ def HTML(x): # noqa - linters dislike upper case function name


can remove this as well

arnavsinghvi11 · 2024-05-05T23:26:42Z

dspy/evaluate/evaluate.py

@@ -29,17 +34,40 @@ def HTML(x):


 class Evaluate:
+ """
+ Reification of the process of evaluating a model. Invoked using its


the evaulate class is for a DSPy program, not a model. Also, can the wording be simplified? :)

Not sure what you mean by simplifying the wording. I can't think of a synonym for "reification," if that's what you mean.

Class representing the process of evaluating a DSpy program.

Would that be better?

dspy/evaluate/evaluate.py

arnavsinghvi11 · 2024-05-05T23:32:50Z

dspy/evaluate/evaluate.py

+ score: float, results: list of Prediction
+ if `return_all_scores` is False and `return_outputs` is True.
+ score: float
+ if both flags are false


keep consistent with phrasing in 222

Good catch. Fixed this. Also one of the lines was wrong, and I added a check (to produce a more understandable error message when required arguments are missing).

arnavsinghvi11 · 2024-05-05T23:33:08Z

dspy/evaluate/evaluate.py

@@ -132,8 +257,10 @@ def wrapped_program(example_idx, example):

 # increment assert and suggest failures to program's attributes
 if hasattr(program, "_assert_failures"):
+ # noinspection PyProtectedMember


remove comments

arnavsinghvi11 · 2024-05-05T23:33:14Z

dspy/evaluate/evaluate.py

@@ -151,11 +278,14 @@ def wrapped_program(example_idx, example):
 if creating_new_thread:
 del thread_stacks[threading.get_ident()]

- devset = list(enumerate(devset))
- tqdm.tqdm._instances.clear()
+ devset = list(enumerate(devset)) # This actually changes the type of `devset`


remove comments

Fixed, but renamed. See if you are ok with the change, please.

arnavsinghvi11 · 2024-05-05T23:33:51Z

dspy/evaluate/evaluate.py

 df = df.map(truncate_cell)
 else:
- df = df.applymap(truncate_cell)
+ df = df.applymap(truncate_cell) # type: ignore


remove comments

dspy/evaluate/evaluate.py

Docstring for the return value had inconsistent wording and one error. Also added explicit check for missing `metric` and `devset` arguments, since these do not have defaults.

cdowellmdb · 2024-06-11T21:49:01Z

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

rpgoldman · 2024-06-13T19:41:26Z

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

arnavsinghvi11 · 2024-06-15T18:29:52Z

dspy/evaluate/evaluate.py


+# noinspection PyUnresolvedReferences


we don't maintain these comments throughout the repo and it's actually fine to flag the linter and add in the changes within the PR. would prefer to remove them.

arnavsinghvi11 · 2024-06-15T18:30:32Z

dspy/evaluate/evaluate.py

+ ==========
+ devset: Iterable[Example]
+ An iterable of Examples
+ metric: Callable, optional


link was for L53 above, corresponding to the metric description

dspy/evaluate/evaluate.py

arnavsinghvi11 · 2024-06-15T18:35:57Z

dspy/evaluate/evaluate.py

+ self,
+ program,
+ metric: Optional[Callable] = None,
+ devset: Optional[Iterable] = None, # Needs more specific type, if possible


I think the latterUnion[List[bool], List[float]] makes sense for this!

arnavsinghvi11 · 2024-06-15T18:36:08Z

dspy/evaluate/evaluate.py

 program._assert_failures += dspy.settings.get("assert_failures")
 if hasattr(program, "_suggest_failures"):
+ # noinspection PyProtectedMember


flagging the comments

arnavsinghvi11 · 2024-06-15T18:36:11Z

dspy/evaluate/evaluate.py

- devset = list(enumerate(devset))
- tqdm.tqdm._instances.clear()
+ ndevset: List[int, Example] = list(enumerate(devset))
+ # noinspection PyProtectedMember


flagging the comments

arnavsinghvi11 · 2024-06-15T18:39:39Z

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.

rpgoldman · 2024-06-17T14:24:49Z

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_failures to 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.

Add docstrings and type hints.

9ae535d

rpgoldman added 3 commits April 30, 2024 10:12

All typing issues fixed EXCEPT overloads.

6abcd05

Format changes from linters.

63618ff

Fix __call__ type hints.

778c56d

With kind help from the mypy folks, fixed the overload hints for the `__call__` method on Evaluate.

rpgoldman changed the title ~~Add docstrings and type hints.~~ Add docstrings and type hints to Evaluate class. May 2, 2024

rpgoldman marked this pull request as ready for review May 2, 2024 16:45

Clarify the meaning of display_table parameter.

f3d6448

arnavsinghvi11 requested changes May 5, 2024

View reviewed changes

arnavsinghvi11 mentioned this pull request May 6, 2024

Make dspy.Evaluate Typed and DSPy Only (No dsp) #889

Open

rpgoldman added 5 commits May 6, 2024 09:25

Drop explanation of "noqa" comment.

3dba26b

Fix incomplete parameter description.

f19b803

Fix return_outputs description.

e90105a

Fix docstring, add error check.

3b0fa20

Docstring for the return value had inconsistent wording and one error. Also added explicit check for missing `metric` and `devset` arguments, since these do not have defaults.

Rename devset after enumerating.

fe9cf2c

arnavsinghvi11 requested changes Jun 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docstrings and type hints to Evaluate class. #935

Add docstrings and type hints to Evaluate class. #935

rpgoldman commented Apr 30, 2024

rpgoldman commented Apr 30, 2024

rpgoldman commented May 2, 2024

arnavsinghvi11 May 5, 2024

rpgoldman May 6, 2024

arnavsinghvi11 Jun 15, 2024

arnavsinghvi11 May 5, 2024

arnavsinghvi11 May 5, 2024

arnavsinghvi11 May 5, 2024

rpgoldman May 6, 2024

arnavsinghvi11 May 5, 2024

rpgoldman May 6, 2024

arnavsinghvi11 May 5, 2024

arnavsinghvi11 May 5, 2024

rpgoldman May 6, 2024

arnavsinghvi11 May 5, 2024

cdowellmdb commented Jun 11, 2024

rpgoldman commented Jun 13, 2024

arnavsinghvi11 Jun 15, 2024

arnavsinghvi11 Jun 15, 2024

arnavsinghvi11 Jun 15, 2024

arnavsinghvi11 Jun 15, 2024

arnavsinghvi11 Jun 15, 2024

arnavsinghvi11 commented Jun 15, 2024

rpgoldman commented Jun 17, 2024

Add docstrings and type hints to Evaluate class. #935

Are you sure you want to change the base?

Add docstrings and type hints to Evaluate class. #935

Conversation

rpgoldman commented Apr 30, 2024

rpgoldman commented Apr 30, 2024

rpgoldman commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdowellmdb commented Jun 11, 2024

rpgoldman commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnavsinghvi11 commented Jun 15, 2024

rpgoldman commented Jun 17, 2024