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: Retrieval top n bug #11305

Closed
wants to merge 1 commit into from
Closed

Conversation

ProseGuys
Copy link
Contributor

@ProseGuys ProseGuys commented Dec 3, 2024

Summary

Sorry, I made a stupid mistake and caused a bad impact on your, this is the revised submission

Close #11300
Close #11068

Screenshots

Before: After:
... ...

Checklist

Important

Please review the checklist below before submitting your pull request.

  • This change requires a documentation update, included: Dify Document
  • I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.
  • I ran dev/reformat(backend) and cd web && npx lint-staged(frontend) to appease the lint gods

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. 🐞 bug Something isn't working labels Dec 3, 2024
@crazywoola crazywoola requested review from laipz8200 and JohnJyong and removed request for laipz8200 December 3, 2024 13:29
@laipz8200
Copy link
Member

Actually, I think we should find a better way to implement this feature.

In the implementation of this PR, if we set the environment variable, the settings we set in the web view will not work.

@nadirvishun
Copy link

Actually, I think we should find a better way to implement this feature.

In the implementation of this PR, if we set the environment variable, the settings we set in the web view will not work.

I think this design would be better:
image

@ProseGuys
Copy link
Contributor Author

Actually, I think we should find a better way to implement this feature.
In the implementation of this PR, if we set the environment variable, the settings we set in the web view will not work.

I think this design would be better: image
I agree with your point of setting on the page side, but there seems to be a problem with the design in the picture. If I choose the other two retrieval methods besides hybrid retrieval, top_n and top_k seem to be unable to be set.

@ProseGuys
Copy link
Contributor Author

Actually, I think we should find a better way to implement this feature.

In the implementation of this PR, if we set the environment variable, the settings we set in the web view will not work.

First of all, I apologize to your team for my mistake, but this temporary solution seems to work in my project, and I also agree with @nadirvishun ‘s opinion of view. Does diffy have any plans to introduce the top_n parameter on the page side?

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Dec 4, 2024
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Dec 4, 2024
@laipz8200
Copy link
Member

I've pinged our team to review this feature request, let's discuss #11068.

@laipz8200
Copy link
Member

We believe this feature is not essential. Please refer to #11068 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
3 participants