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: upload less than 20 files via sync endpoints #142

Merged
merged 25 commits into from
Dec 4, 2023

Conversation

ArzelaAscoIi
Copy link
Member

@ArzelaAscoIi ArzelaAscoIi commented Nov 24, 2023

Related Issues

  • reduce unneccessary upload sessions for small chunks of files

Proposed Changes?

  • use sync upload if there are less than 20 files in a session/folder
  • bump to python 3.10

How did you test it?

  • unit tests
  • integration tests

Checklist

  • I have updated the referenced issue with new insights and changes
  • If this is a code change, I have added unit tests
  • I've used the conventional commit specification for my PR title
  • I updated the docstrings
  • If this is a code change, I added meaningful logs and prepared Datadog visualizations and alerts

Copy link

github-actions bot commented Nov 24, 2023

Coverage report

The coverage rate went from 92.9% to 95.11% ⬆️
The branch rate is 88%.

95.45% of new lines are covered.

Diff Coverage details (click to unfold)

deepset_cloud_sdk/_service/files_service.py

93.02% of new lines are covered (95.67% of the complete file).
Missing lines: 166, 167, 168

deepset_cloud_sdk/_api/files.py

100% of new lines are covered (92.63% of the complete file).

@ArzelaAscoIi ArzelaAscoIi changed the title faet: upload less than 100 files via sync endpoints faet: upload less than 20 files via sync endpoints Nov 27, 2023
@ArzelaAscoIi ArzelaAscoIi changed the title faet: upload less than 20 files via sync endpoints feat: upload less than 20 files via sync endpoints Nov 27, 2023
Copy link
Collaborator

@rjanjua rjanjua left a comment

Choose a reason for hiding this comment

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

a few minor points, but really great PR, and I can see there was a lot of effort on the testing!

deepset_cloud_sdk/_service/files_service.py Show resolved Hide resolved
deepset_cloud_sdk/_service/files_service.py Show resolved Hide resolved
tests/unit/api/test_files.py Show resolved Hide resolved
@ArzelaAscoIi
Copy link
Member Author

I was actually also thinking about using the strategy pattern here: https://refactoring.guru/design-patterns/strategy/python/example. With that it would be possible to have two strategies (sync and batch) that are generated based on the number of files. Buuut i think its not for the effort for now 🙈 let's keep it in mind!

Is that fine for you ?

@ArzelaAscoIi ArzelaAscoIi added this pull request to the merge queue Dec 4, 2023
Merged via the queue into main with commit 119db5c Dec 4, 2023
9 checks passed
@ArzelaAscoIi ArzelaAscoIi deleted the feat/uploadLT100filesSync branch December 4, 2023 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants