Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(agent, forge): Silence Pydantic v2 protected namespace model_ warning #7340

Merged
merged 2 commits into from
Jul 12, 2024

Conversation

kcze
Copy link
Contributor

@kcze kcze commented Jul 7, 2024

Background

After updating to Pydantic v2 Pydantic complains about model fields with model_ prefix as it's a Pydantic protected namespace:

UserWarning: Field "model_info" has conflict with protected namespace "model_".

To silence this warning fields need to be renamed or namespace need to be removed from protected_namespaces.

Changes πŸ—οΈ

This PR renames model_* fields to llm_* in affected models.

PR Quality Scorecard ✨

  • Have you used the PR description template?   +2 pts
  • Is your pull request atomic, focusing on a single change?   +5 pts
  • Have you linked the GitHub issue(s) that this PR addresses?   +5 pts
  • Have you documented your changes clearly and comprehensively?   +5 pts
  • Have you changed or added a feature?   -4 pts
    • Have you added/updated corresponding documentation?   +4 pts
    • Have you added/updated corresponding integration tests?   +5 pts
  • Have you changed the behavior of AutoGPT?   -5 pts
    • Have you also run agbenchmark to verify that these changes do not regress performance?   +10 pts

@kcze kcze requested a review from Pwuts July 7, 2024 10:21
@kcze kcze requested a review from a team as a code owner July 7, 2024 10:21
@kcze kcze requested review from majdyz and removed request for a team July 7, 2024 10:21
Copy link

netlify bot commented Jul 7, 2024

βœ… Deploy Preview for auto-gpt-docs ready!

Name Link
πŸ”¨ Latest commit 63da9c4
πŸ” Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/668bbf563eb790000880fa48
😎 Deploy Preview https://deploy-preview-7340--auto-gpt-docs.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Jul 7, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 54.21%. Comparing base (08612cc) to head (63da9c4).
Report is 11 commits behind head on master.

Files Patch % Lines
.../forge/components/action_history/action_history.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7340   +/-   ##
=======================================
  Coverage   54.21%   54.21%           
=======================================
  Files         122      122           
  Lines        6875     6875           
  Branches      881      881           
=======================================
  Hits         3727     3727           
  Misses       3015     3015           
  Partials      133      133           
Flag Coverage Ξ”
Linux 53.96% <80.00%> (ΓΈ)
Windows 50.74% <80.00%> (ΓΈ)
autogpt-agent 34.01% <100.00%> (ΓΈ)
forge 58.63% <75.00%> (ΓΈ)
macOS 53.26% <80.00%> (ΓΈ)

Flags with carried forward coverage won't be shown. Click here to find out more.

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

Copy link
Member

@Pwuts Pwuts left a comment

Choose a reason for hiding this comment

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

How about we replace all of these with llm_*? Slightly bigger refactor, but no need to infringe on the model_* namespace. I'm not sure why Pydantic made it a protected namespace, but I'd rather not disable that (default) protection if we don't need to.

@kcze
Copy link
Contributor Author

kcze commented Jul 7, 2024

How about we replace all of these with llm_*

I considered that but yes it's more changes. I'll replace field and type names.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jul 8, 2024
@kcze kcze requested a review from Pwuts July 8, 2024 10:30
Copy link
Member

@Pwuts Pwuts left a comment

Choose a reason for hiding this comment

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

Nice and clean. LGTM!

@Pwuts Pwuts merged commit bffb92b into master Jul 12, 2024
29 checks passed
@Pwuts Pwuts deleted the kpczerwinski/open-1379-rename-model_-fields-in-models branch July 12, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoGPT Agent documentation Improvements or additions to documentation Forge size/m
Projects
Status: βœ… Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants