jxnl / instructor

structured outputs for llms
https://python.useinstructor.com/
MIT License
7.56k stars 602 forks source link

Async Retry is broken #628

Closed lazyhope closed 4 months ago

lazyhope commented 4 months ago

What Model are you using?

Describe the bug e.attempt_number doesn't actually exists, the following lines would raise AttributeError: 'pydantic_core._pydantic_core.ValidationError' object has no attribute 'attempt_number'

https://github.com/jxnl/instructor/blob/2b80e13215fbe0ee971316a6cda09136f1c10a79/instructor/retry.py#L230-L239

To Reproduce

Reproduce in the interpreter:

from typing import Annotated

import instructor
from anthropic import AsyncAnthropic
from pydantic import BaseModel, field_validator

client = instructor.from_anthropic(AsyncAnthropic(), mode=instructor.Mode.ANTHROPIC_JSON, model="claude-3-haiku-20240307")

class User(BaseModel):
    name: str

    @field_validator("name")
    def validate_gender(cls, v, info):
        raise ValueError("You will never succeed")

await client.chat.completions.create(
    max_tokens=4096,
    temperature=0,
    messages=[{"role": "user", "content": "Extract Jason is 25 years old"}],
    response_model=User,
)
lazyhope commented 4 months ago

Also Jason @jxnl could you please explain the purpose of the following lines?

https://github.com/jxnl/instructor/blob/2b80e13215fbe0ee971316a6cda09136f1c10a79/instructor/retry.py#L227-L234

It seems like these lines are redundant, as tenacity will always catch any raised exceptions for us

dosubot[bot] commented 4 months ago

It looks like you've identified a couple of significant issues with the async retry mechanism and raised a question about the purpose of certain lines of code in the retry.py file. For the bug related to e.attempt_number not existing, this indeed seems like an oversight that could lead to an AttributeError during exception handling in retries. Your detailed description and reproduction steps are very helpful for pinpointing the problem.

Regarding your question about the seemingly redundant lines in the retry.py file, you're correct that tenacity is designed to handle retries and exceptions internally, which might make these lines appear unnecessary. However, there could be specific reasons for their inclusion, such as additional logging, custom handling, or a workaround for a particular scenario that's not immediately obvious.

@jxnl, this one's a bit out of my wheelhouse, could you step in and provide your guidance?

#### Details **About Dosu** This response is meant to be useful and save you time. It is **not meant to be a precise solution**, but rather a starting point for your own research. Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

To continue the conversation, mention @dosu.

lazyhope commented 4 months ago

Add a fix to this in my last PR #622

lazyhope commented 4 months ago

@jxnl Hi Jason thanks for reviewing my PR and merge it. Before this issue is completely closed could I ask why is there a InstructorRetryError inside the try clause, it doesn't seem necessary to raise it until the except clause?

jxnl commented 4 months ago

Probs me being dumb.

lazyhope commented 4 months ago

😂