Skip to content

Fix message access in infer_requests#9073

Open
YeewahChan wants to merge 1 commit intomodelscope:mainfrom
YeewahChan:main
Open

Fix message access in infer_requests#9073
YeewahChan wants to merge 1 commit intomodelscope:mainfrom
YeewahChan:main

Conversation

@YeewahChan
Copy link
Copy Markdown

PR type

  • Bug Fix
  • New Feature
  • Document Updates
  • More Models or Datasets Support

PR information

Due to the definition of swift.infer_engine.protocol.InferRequest, the swift.pipelines.sampling.distill_sampler.OpenAIEngine may have to use a different way to access the infer_requests.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies the distill_sampler.py file to use attribute-style access for messages instead of dictionary-style access. However, a critical issue was identified as infer_request is a dictionary, and this change will lead to a runtime AttributeError. It is recommended to revert this change.

completion = self.client.chat.completions.create(
model=self.model,
messages=infer_request['messages'],
messages=infer_request.messages,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This change from dictionary-style access (infer_request['messages']) to attribute-style access (infer_request.messages) will cause a runtime AttributeError. The infer_requests list passed to this infer method is populated with dictionaries in DistillSampler.generate, not InferRequest objects. As infer_request is a dictionary, attribute access is not possible.

Please revert this change to use infer_request['messages'] to avoid breaking the code.

Suggested change
messages=infer_request.messages,
messages=infer_request['messages'],

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant