Repository navigation
Build fails due to OpenProcessing rate limits (429) #955
Description
Activity
Thanks for logging this @perminder-17 (also cc @davepagurek since this maybe a repeat from this potentially related prior issue: #955)
I've marked is as "Critical" because it blocks many outstanding PRs.I will investigate the following: (1) not doing excessive OP API calls in CI (just 1 confirm it works) and (2) ensure there's a reasonable fallback on the site itself.
Based on the code in https://github.com/processing/p5.js-website/pull/838/files, it looks like it should only hit that error if we're trying to load a sketch that's not in one of our default curations which we load in bulk once. It looks like this could happen if:
- We are calling it with different params so it's not cached. Looks like we always call with the same params though.
- Something about
memoizeisn't working, e.g. we're building pages in parallel where it used to be serial and the separate processes aren't using the same cache. We could try having a manual build script that pulls the data and writes the result to a json file that we check into git, so that we aren't constantly querying OP on every build - We're requesting a sketch that isn't in a curation (not sure where this would come from in our source code though)
- Something changed on OP's side. Do we see the failures starting after a change on our end?
I don't know what the specific issue is, but the second bullet would probably address it regardless if we make querying OP a manual step. If we aren't sure what the cause is, that could be a way forward? e.g.:
- Make a script that pulls all curation sketches + their source code into a big JSON file, e.g.
npm run build:sketches - Check in initial results in git
- Update code to read from that file rather than from a
fetchresult - Update our docs to say that when we want to update content from OP, we have to rerun the script
The OpenProcessing API appears to have rate limiting for consecutive requests within a short time period.
When I make several consecutive requests, I get this response:/GET https://openprocessing.org/api/curation/87649/sketches { "success": false, "message": "Our Replicant Detection Division specialist Deckard believes that you repeated this action too many times for a human. Please try again in 47 seconds.", "object": null, "code": 429, "level": "info", "status": 429 }After debugging, I found that the 429 errors are coming from the individual sketch API calls in the
getSketch()function.
This part was being called for almost every sketch page, which means that memoizedSketch isn't working.p5.js-website/src/api/OpenProcessing.ts
Lines 122 to 129 in da0dfb5
Even though we're fetching from the same
getCurationSketches()source, it's not finding matches in memoizedSketch because there's a type mismatch: thevisualIDfrom OpenProcessing'sgetCurationSketches()items comes as a number type, but the id parameter passed to getSketch fromSketchLayoutis a string type.When I added this logging code and checked the types, I saw:
export const getSketch = memoize( async (id: string): Promise<OpenProcessingSketchResponse> => { // check for memoized sketch in curation sketches const curationSketches = await getCurationSketches(); console.log('parameter type:', typeof id); console.log('data type', typeof curationSketches[0].visualID); console.log('is same?', id === curationSketches[0].visualID);parameter type: string OP data type number is same? falseSo the issue was in the comparison part:
const memoizedSketch = curationSketches.find((el) => el.visualID === id)
I think This can be easily fixable with a simple type conversion to enable proper caching.Could you assign this issue to me? I'd like to solve it.
Reacted by Perminder SinghAah....you're correct. We are using string instead of using number so,
el.visualID === idis always false. Cache will miss everytime. I was debugging it but didn't noticed we are using a number. Probably your solution should fix the issue. Thanks for catching this. I'll assign you, Note: That's a critical bug which blocks other PRs merging so has a higher priority.Thanks for debugging this.
Reacted by Jang Junseok and Divyansh SrivastavaThanks for the fix @junseok44, I've merged it. I agree with your suggestion for followup PRs, especially not just logging the error but more meaningfully excluding invalid IDs and ensuring fallback behaviors. Do you want to keep working on this?
Thanks! I'd like to continue working on this.
I believe excluding invalid IDs during static page generation (ingetStaticPaths()) should solve most problems.
Do you think we need to add any additional fallback behaviors beyond that?Great, thanks for your work @junseok44 .
I believe excluding invalid IDs during static page generation (in getStaticPaths()) should solve most problems.
Yes, sounds good
Do you think we need to add any additional fallback behaviors beyond that?
I don't think so, but please check both website function and test behavior. Failing OP API calls should still result in failing tests.
If you have any other ideas on improving tests / error logging, please feel free to do that. Some other optional thoughts: for example if the "rate limit" error is reached, then no further calls are made past the first failing one (not sure if this is already the behavior). Potentially, there should also be an automated check that caching is working / that not "too many" API calls are made, to prevent future regression? (When I was initially thinking about reducing number of calls / adding a fallback, it was more from the perspective of what to do if really too many calls are needed; it's great that there was a clear bugfix.) Those are all optional ideas, you're welcome to include what you think is reasonable in your PR.
EDIT: "Critical" tag has been removed, since the time-sensitive part of this issue has been addressed.
Reacted by Jang JunseokI've worked on the error handling and test code for this PR.
Currently, getCurationSketches() and getSketch() only use console.error when a fetch fails. However, I think it's better to explicitly throw Error to fail the build rather than returning invalid data that could cause build errors during processing.
p5.js-website/src/api/OpenProcessing.ts
Lines 50 to 52 in 80c9390
comment on this line mentioned using console.error instead of throw Error to prevent caching failed results, but since throw Error would fail the entire build anyway, i thought caching failed values wouldn't matter much.
(While this shouldn't happen currently) For cases like getSketch() where individual pages might have cache misses and need to fetch data one by one, I was debating how to handle failures. For now, I decided to fail the entire build by throwing error in getSketch. but we could also make it skip just that specific page if needed.
By the way, in our current CI/CD pipeline, the build runs independently from the tests. Since we're now testing whether the OP API is properly cached, shouldn't the build happen only after the tests pass?
Reacted by kitThanks @junseok44 ! I'll close this as the linked PRs are closed.
Metadata
Metadata
Assignees
Labels
Type
Projects
- Status
Most appropriate sections of the p5.js website?
Home
What is your operating system?
Linux
Web browser and version
Firefox
Actual Behavior
Our site build sometimes fails when pages fetch data from OpenProcessing. The API returns 429 Too Many Requests, which makes our code expect arrays/strings but get nothing, causing crashes.
Here's the CI failure : #954 when I tried uploading images. You can see the errors in the PR https://github.com/processing/p5.js-website/actions/runs/17451097533/job/49557683435?pr=954
Expected Behavior
There should be no crashes when OP returns 429 or bad JSON.
Steps to reproduce
No response
Would you like to work on the issue?
No, I am not very confident with typeScript and CI testings, but a possible solution would be (during tests and local builds, don’t call OpenProcessing at all)