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

feat: add support muxing rules #311

Merged
merged 32 commits into from
Feb 14, 2025
Merged

feat: add support muxing rules #311

merged 32 commits into from
Feb 14, 2025

Conversation

peppescg
Copy link
Collaborator

@peppescg peppescg commented Feb 13, 2025

Add Muxing rules support:

  • drag n drop rules
  • add dropdown with grouped models by provider name
Kapture.2025-02-13.at.21.08.57.mp4

Remove old queries

We had an issue caused byt the fact that after rename the workspace name we didn't remove the old staled query based on the old workspace name, this caused an API fetch with old workspace name that return a 404 for custom-instructions and muxing.
I created a util func that received as input the queryClient and a list of queriesFns removing all the old queries after the workspace name mutation.

BEFORE

Kapture.2025-02-13.at.11.40.12.mp4

AFTER

Kapture.2025-02-13.at.11.41.04.mp4

Copy link

stacklok-cloud-staging bot commented Feb 13, 2025

Minder Vulnerability Report ✅

Minder analyzed this PR and found it does not add any new vulnerable dependencies.

Vulnerability scan of 7b0e5f61:

  • 🐞 vulnerable packages: 0
  • 🛠 fixes available for: 0

@peppescg peppescg linked an issue Feb 13, 2025 that may be closed by this pull request
@peppescg peppescg changed the title Issues/278 feat: support muxing rules Feb 13, 2025
@peppescg peppescg changed the title feat: support muxing rules feat: support muxing rules Feb 13, 2025
@peppescg peppescg changed the title feat: support muxing rules feat: add support muxing rules Feb 13, 2025
@peppescg peppescg marked this pull request as ready for review February 13, 2025 16:12
@peppescg peppescg self-assigned this Feb 13, 2025
@peppescg
Copy link
Collaborator Author

@danbarr could you take a look to this PR, I would like to have your feedback on this?

@danbarr
Copy link
Collaborator

danbarr commented Feb 13, 2025

Thanks @peppescg

  • The new model selection with search works great! Only note, the provider headings feel a little small relative to the model entries, the ones lower down the list can get a little lost when scrolling.
  • It would be helpful to have an example or two of filters, either in the filter entry box or below - it's not clear to me what format they should be in - simple wildcards, glob patterns, regex, can I comma-separate multiple patterns, etc?
  • Instead of an empty filter as the catch-all (what happens if I don't leave an empty one?), is it possible to have a fixed "Default" rule that's always at the bottom (no re-ordering drag handle)?
  • The error message if I add a rule but leave the model empty is "Provider does not exist", I assume that's coming from the backend, might be nice to validate on the form that each rule has a model selected when you click Save?
  • I notice you removed the "Select the model you would like to use in this workspace. This section applies only if you are using the MUX endpoint." line - I think this might still be useful (with a tweak to "model(s)")?

@coveralls
Copy link
Collaborator

coveralls commented Feb 13, 2025

Pull Request Test Coverage Report for Build 13328630689

Details

  • 102 of 121 (84.3%) changed or added relevant lines in 14 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 68.087%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/features/providers/hooks/use-invalidate-providers-queries.ts 0 1 0.0%
src/features/workspace/components/workspace-muxing-model.tsx 17 19 89.47%
src/features/workspace/hooks/use-muxing-rules-form-workspace.ts 19 23 82.61%
src/features/workspace/components/workspace-models-dropdown.tsx 21 26 80.77%
src/components/SortableArea.tsx 3 10 30.0%
Totals Coverage Status
Change from base Build 13328560300: 0.1%
Covered Lines: 823
Relevant Lines: 1137

💛 - Coveralls

model: modelName,
provider_id,
})
setIsOpen(false)
Copy link
Member

Choose a reason for hiding this comment

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

@alex-mcgovern same question as always 🤔

I guess in this case it's a bit more tricky, but should we try to avoid the controlled state here somehow?

@peppescg peppescg merged commit 38f2ecc into main Feb 14, 2025
8 checks passed
@peppescg peppescg deleted the issues/278 branch February 14, 2025 11:56
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.

[Task]: mux v2, support file type and file name rules
5 participants