Skip to content

Conversation

@jasonfish568
Copy link
Contributor

@jasonfish568 jasonfish568 commented Dec 30, 2024

  • Added AnnotationReplyActionApi, AnnotationListApi, AnnotationUpdateDeleteApi
  • Added GET and PATCH endpoints to DatasetApi
  • Applied current_user in validate_app_token just like what it is done in validate_dataset_token
  • Added available model API

Close #11727

Summary

Basically copied the functions from console to the service api. Also added clear_partial_member_list() to Dataset Delete endpoint as what has been added in the console api.

I will keep working on this feature until it fulfills my needs. More endpoints will be added.

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

- Added AnnotationReplyActionApi, AnnotationListApi, AnnotationUpdateDeleteApi
- Added GET and PATCH endpoints to DatasetApi
- Applied current_user in validate_app_token just like what it is done in validate_dataset_token

Related to: langgenius#11727
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. 💪 enhancement New feature or request labels Dec 30, 2024
This API endpoint shares the same API token with Dataset API.
It is essential for patching the knowledge base in terms of embedding model.

Related to: langgenius#11727
@jasonfish568 jasonfish568 marked this pull request as draft December 31, 2024 05:43
jasonfish568 added a commit to jasonfish568/dify-docs that referenced this pull request Dec 31, 2024
Because app token is assigned to a specific app, the validate_app_token decorator has already validated the app_id.
The returned app_model is the app targeted. Therefore there is no need to pass app_id separately.
Simply use app_model.id instead. So app_id is removed from the api routes.

Related to: langgenius#11727
@jasonfish568 jasonfish568 marked this pull request as ready for review December 31, 2024 07:11
@dosubot dosubot bot added the 📚 documentation Improvements or additions to documentation label Dec 31, 2024
@crazywoola
Copy link
Member

Please fix the errors in CI.

@jasonfish568
Copy link
Contributor Author

Please fix the errors in CI.

Errors fixed.

@crazywoola
Copy link
Member

Please fix the errors in CI.

Errors fixed.

Please run dev/reformat to resolve the errors in the CI.

@jasonfish568
Copy link
Contributor Author

Please fix the errors in CI.

Errors fixed.

Please run dev/reformat to resolve the errors in the CI.

Sorry for the trouble. Code has now been reformated.

@crazywoola
Copy link
Member

crazywoola commented Jan 3, 2025

The code part LGTM, however you should add according docs to the API Access as well in CN, EN & JA.

@jasonfish568
Copy link
Contributor Author

The code part LGTM, however you should add according docs to the API Access as well in CN, EN & JA.

langgenius/dify-docs#424

The relevant docs have been added and amended outdated part.

I have included both Chinese and English versions. However as I do not speak Japanese, I wouldn't be able to verify the translated version therefore I would wait for a Japanese contributor get it translated in a later time.

@jasonfish568
Copy link
Contributor Author

The code part LGTM, however you should add according docs to the API Access as well in CN, EN & JA.

Could you let me know when this PR will be merged? How does dify team manage PRs? When do PRs ususally get merged?

@crazywoola
Copy link
Member

The code part LGTM, however you should add according docs to the API Access as well in CN, EN & JA.

Could you let me know when this PR will be merged? How does dify team manage PRs? When do PRs ususally get merged?

Hello, for newly added apis, you need to update the docs in mdx files as well.

  • There is a conflict file.
  • Here are the docs I mentioned.
image image

@crazywoola crazywoola requested a review from JohnJyong January 13, 2025 09:54
It is added to Chat App due to Annotation is under Apps, and there is no individual Annotation API doc page. It seems correct to have Annotation APIs put here.
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jan 16, 2025
@jasonfish568
Copy link
Contributor Author

jasonfish568 commented Jan 16, 2025

The code part LGTM, however you should add according docs to the API Access as well in CN, EN & JA.

Could you let me know when this PR will be merged? How does dify team manage PRs? When do PRs ususally get merged?

Hello, for newly added apis, you need to update the docs in mdx files as well.

  • There is a conflict file.
  • Here are the docs I mentioned.

image image

Outcome

  1. Conflict solved
  2. Docs added to the mdx files

Suggestion:

I have always been wondering why there are docs.dify.ai and these API docs in the portal. It does not only confuse developers but also becomes a nightmare for contributors like me. I personally don't know mdx and my VSCode doesn't provide preview for mdx.

It would be a lot better if we can consolidate all docs to the same place without having to prepare duplicate docs all over the places. docs.dify.ai is already a built up platform hosting our docs. using markdown is also a standard way for the community to prepare docs.

Anyway, I'm just a newbie contributor. Dify team might have been preparing a new doc platform. So it's your call.

@jasonfish568
Copy link
Contributor Author

@JohnJyong Happy New Year. Just a follow up on this PR. Please kindly process at your earliest convenience.

@jasonfish568
Copy link
Contributor Author

The code part LGTM, however you should add according docs to the API Access as well in CN, EN & JA.

Could you let me know when this PR will be merged? How does dify team manage PRs? When do PRs ususally get merged?

Hello, for newly added apis, you need to update the docs in mdx files as well.

  • There is a conflict file.
  • Here are the docs I mentioned.

image image

Hi,

I was wondering if you could speed up the merge of this PR? I'm desparately willing to use these APIs for my project.

Thanks

@jasonfishblu
Copy link

@crazywoola Could you please clarify that version 1.0 has got these APIs implemented? The Dify Doc PR got denied due to not planned.

If this is never gonna be a feature in the official package, then I won't be bothered to contribute. My pay rate is too high to be wasted. Very much disappointed that when other contributors spent quite some time but received no response.

@crazywoola
Copy link
Member

crazywoola commented Mar 21, 2025

@crazywoola Could you please clarify that version 1.0 has got these APIs implemented? The Dify Doc PR got denied due to not planned.

If this is never gonna be a feature in the official package, then I won't be bothered to contribute. My pay rate is too high to be wasted. Very much disappointed that when other contributors spent quite some time but received no response.

Sorry for the late response, this is a great feature that we would like to have.

I know that the explanation is rather insipid. The fact is, we had a major infra update this week.

So most of the PRs can not get merged during this time.

Can you resolve the conflicts, I will merged it asap. :)

@jasonfishblu

@crazywoola crazywoola self-requested a review March 21, 2025 11:47
@jasonfishblu
Copy link

Thanks for the reply, I will arrange a time to get the conflicts sorted.

@crazywoola
Copy link
Member

Thanks for the reply, I will arrange a time to get the conflicts sorted.

Cool Thanks, sorry for the inconvience.

@jasonfish568
Copy link
Contributor Author

Thanks for the reply, I will arrange a time to get the conflicts sorted.

Cool Thanks, sorry for the inconvience.

Hi Crazywoola,

I have managed to get the conflicts resolved. New doc added by other contributors did get honoured so no info missed.

Please help get this PR merged to prevent further conflicts at your earliest convenience.

Very much appreciated!

Jason

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 7, 2025
@crazywoola crazywoola merged commit fd44394 into langgenius:main Apr 7, 2025
7 checks passed
@crazywoola crazywoola requested a review from Copilot April 7, 2025 03:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 11 changed files in this pull request and generated 1 comment.

Files not reviewed (6)
  • web/app/(commonLayout)/datasets/template/template.en.mdx: Language not supported
  • web/app/(commonLayout)/datasets/template/template.zh.mdx: Language not supported
  • web/app/components/develop/template/template.zh.mdx: Language not supported
  • web/app/components/develop/template/template_advanced_chat.en.mdx: Language not supported
  • web/app/components/develop/template/template_advanced_chat.zh.mdx: Language not supported
  • web/app/components/develop/template/template_chat.en.mdx: Language not supported
Comments suppressed due to low confidence (1)

api/controllers/service_api/dataset/dataset.py:156

  • The retrieval and update for 'partial_member_list' is performed twice in the GET method which appears redundant. Consider removing one of the duplicate calls to streamline the response assembly.
if data.get("permission") == "partial_members":

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

Labels

📚 documentation Improvements or additions to documentation 💪 enhancement New feature or request lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Annotation API

3 participants