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

🔧 Increase max and default page limits #629

Merged
merged 5 commits into from
Aug 14, 2023
Merged

Conversation

znatty22
Copy link
Member

@znatty22 znatty22 commented Aug 8, 2023

Motivation

The kf-api-study-creator API often queries the Dataservice when it syncs studies between Dataservice and itself. Unfortunately there is a bug in study creator where it only ever requests the first 100 studies since this is the maximum # of resources you can fetch in a single request.

Additionally, several other API consumers have long been requesting the maximum page size be increased from 100 to something larger so that they can send less requests.

Approach

  • Increase the maximum page size from 100 to 1000
  • Increase the default page size from 10 to 100

@znatty22 znatty22 added the refactor Something needs to be done better label Aug 8, 2023
@znatty22 znatty22 self-assigned this Aug 8, 2023
@znatty22 znatty22 requested a review from a team as a code owner August 8, 2023 19:53
@znatty22 znatty22 requested a review from a team as a code owner August 9, 2023 16:04
@znatty22 znatty22 force-pushed the increase-limit-max branch 2 times, most recently from 516859b to 115070b Compare August 9, 2023 20:18
@znatty22 znatty22 changed the title 🔧 Increase limit max 🔧 Increase max and default page limits Aug 10, 2023

# Important but *temporary*
# See https://github.com/yaml/pyyaml/issues/724
pip install "cython<3.0.0" && pip install --no-build-isolation "pyyaml==5.4.0"
Copy link
Member

@devbyaccident devbyaccident Aug 14, 2023

Choose a reason for hiding this comment

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

That's an annoying issue. Is there something that we need pyyaml at 5.4.0 for, or can we take a newer version like pyyaml<6.0? Would rather not hard-code a specific version to change later if we can avoid it.

Also, yaml/pyyaml#724 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

@devbyaccident Lol that image is so accurate.

We've always had pyyaml pinned to 5.4.0, and I believe its bc a while ago, something broke and caused us to pin it. So I don't want to change anything that we don't absolutely have to in this PR. I can try changing it in another PR so the change isolated

Copy link
Member

Choose a reason for hiding this comment

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

Gotchya, that makes sense. Ok, I'm happy to give it the 👍 , but I can't speak as much for the non-docker/package changes. Would still like someone from @kids-first/dataservice-reviewers to take a look.

Copy link
Member

@devbyaccident devbyaccident left a comment

Choose a reason for hiding this comment

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

👍

@znatty22 znatty22 merged commit 9d38045 into master Aug 14, 2023
1 check passed
@znatty22 znatty22 deleted the increase-limit-max branch August 14, 2023 16:13
znatty22 added a commit that referenced this pull request Aug 14, 2023
## Release 1.21.0

### Summary

- Emojis: 🔧 x1
- Categories: Ops x1

### New features and changes

- [#629](#629) - 🔧 Increase max and default page limits - [9d38045](9d38045) by [znatty22](https://github.com/znatty22)
@znatty22 znatty22 mentioned this pull request Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Something needs to be done better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants