Skip to content

Remove old v2 model fields frontend code #1455

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Remove old v2 model fields frontend code #1455

wants to merge 1 commit into from

Conversation

jonahjung22
Copy link
Collaborator

References

Resolves #1453 .

Code changes

  • Removed legacy model-fields.tsx component and associated types from packages/jupyter-ai/src/components/settings/
  • Renamed component from ModelParametersInput to ModelArgumentsInput since parameters are function variables and arguments are the actual input values)
  • Removed unnecessary comments

@jonahjung22 jonahjung22 requested a review from dlqqq August 12, 2025 21:59
@dlqqq
Copy link
Member

dlqqq commented Aug 12, 2025

It looks like this is targetting the main branch instead of the litellm branch. Can you update the target branch?

@jonahjung22 jonahjung22 changed the base branch from main to litellm August 12, 2025 23:29
dlqqq
dlqqq reviewed Aug 13, 2025
View reviewed changes
Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@jonahjung22 Thank you for working on this so quickly Jonah! Left a small comment that clarifies the original issue.

@dlqqq dlqqq added the maintenance Change related to maintenance of the repository label Aug 13, 2025
dlqqq
dlqqq approved these changes Aug 13, 2025
View reviewed changes
Copy link
Member

@dlqqq dlqqq left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for the help Jonah! Changes look good. 👍

#1459 will fix the CI on the litellm branch. It would be good to wait until #1459 is merged, rebase this PR, then let the CI pipeline verify that your PR doesn't break anything before merging. Let me know if I can help clarify anything about this process.

dlqqq
dlqqq reviewed Aug 18, 2025
View reviewed changes
Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

This PR looks like it is including a lot of duplicate commits from the litellm branch. My guess is something went wrong while rebasing.

You are free to try to fix this branch, but it may be easier to open a new PR with your changes against the latest litellm branch.

@dlqqq
Copy link
Member

dlqqq commented Aug 18, 2025

It seems like #1456 includes these changes, so the easiest option may be to close this PR in favor of #1456.

@dlqqq dlqqq mentioned this pull request Aug 18, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlqqq dlqqq dlqqq approved these changes

Assignees
No one assigned
Labels
maintenance Change related to maintenance of the repository
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove old v2 model fields frontend code
2 participants
@jonahjung22 @dlqqq