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

Add lazy loading for GitHub API calls #33

Merged
merged 14 commits into from
Jan 30, 2024
Merged

Add lazy loading for GitHub API calls #33

merged 14 commits into from
Jan 30, 2024

Conversation

MillironX
Copy link
Member

Sorry if it seems like I'm polishing the silverware on the Titanic right now...but I was discovered some additional features while playing around in the code base that I thought would be helpful as a holdover for the time while I figure out how to completely switch the API.

There are some configuration and optimization changes that help make API pagination more efficient in this PR, but those are just prerequisites for the main feature: lazy loading. On the master branch right now, the action makes $N / 30$ API calls, where $N$ is the number of total Nextflow releases, no matter which release you are requesting. By implementing this PR, the action only calls the API until it finds a matching release (or can confirm that the release meets all requirements, in the case of latest-everything). This cuts the number of API calls down to a third of their previous value (and that's for the old Nextflow versions that aren't returned on the first call).

Number of API calls performed by act -j test

master This PR
157 45

Although the types are different, the intent is the same, so I think the
name is fitting.
Now that we are lazy-loading the API pages, OctokitWrapper is handling the
conversion to NextflowRelease in one function call. This means we can't test
the API schema. That should be fine as we pinned the API version. We can
test that the resulting objects for validity, so do that.
The pagination changes require Node.js 18 or greater, so update the test
apparatus to use the latest LTS version of Node.
Copy link
Collaborator

@edmundmiller edmundmiller left a comment

Choose a reason for hiding this comment

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

Sorry if it seems like I'm polishing the silverware on the Titanic right now

I think you just said my new favorite phrase 😆

Awesome stuff! I don't think this is useless, it's an itteration! Great work, and I hope it was fun!

@MillironX MillironX merged commit ba0bc84 into nf-core:master Jan 30, 2024
16 checks passed
@MillironX MillironX deleted the feature/lazy-pagination branch January 30, 2024 16:42
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.

2 participants