Skip to content

roman/increase ingest tests num processes#1500

Merged
badGarnet merged 9 commits into
mainfrom
roman/parallel-ingest-jobs
Sep 26, 2023
Merged

roman/increase ingest tests num processes#1500
badGarnet merged 9 commits into
mainfrom
roman/parallel-ingest-jobs

Conversation

@rbiseck3
Copy link
Copy Markdown
Contributor

Description

In an effort to speed up the ingest tests, bumping the num if processes to the max on the system for each

@rbiseck3 rbiseck3 force-pushed the roman/parallel-ingest-jobs branch 5 times, most recently from 99ca269 to 3ed1105 Compare September 25, 2023 14:32
@rbiseck3 rbiseck3 force-pushed the roman/parallel-ingest-jobs branch from 3ed1105 to 70b9b9c Compare September 26, 2023 13:34
@rbiseck3 rbiseck3 marked this pull request as ready for review September 26, 2023 13:37
@ahmetmeleq
Copy link
Copy Markdown
Contributor

Do we need changelog items for changes in test scripts and ci setup? We don't have any changelog items right now

@ahmetmeleq
Copy link
Copy Markdown
Contributor

ahmetmeleq commented Sep 26, 2023

The runner has not picked the ingest tests yet (for 30 mins), is it expected / was it happening before?

If it wasn't happening before, may this be related to changing the runner type (maybe the new runner has higher demand or lower supply)

If this kind of a supply / demand issue occurs often, it might be better to stay in a more available / abundant runner type

@rbiseck3
Copy link
Copy Markdown
Contributor Author

@ahmetmeleq, was testing the larger runner type based on this previous PR: #1448. But I agree, if the wait for the runner negates the increase in testing speed, might as well stay with the smaller runner type.

Regarding CHANGELOG updates, we're not including any notes when it comes to only the testing approach, so just a version bump is all that's needed.

@ryannikolaidis
Copy link
Copy Markdown
Contributor

before we merge, worth running tests a few times to validate the point on wait times?

@rbiseck3
Copy link
Copy Markdown
Contributor Author

@ryannikolaidis, you can see the increase by looking at the CI run in this PR (https://github.com/Unstructured-IO/unstructured/actions/runs/6313481314/job/17141787146?pr=1500) 15min, to one on any other PR or main: (https://github.com/Unstructured-IO/unstructured/actions/runs/6313633584/job/17143422610?pr=1524) 22min. Not exactly a 2x improvement but something.

@ryannikolaidis
Copy link
Copy Markdown
Contributor

@ryannikolaidis, you can see the increase by looking at the CI run in this PR (https://github.com/Unstructured-IO/unstructured/actions/runs/6313481314/job/17141787146?pr=1500) 15min, to one on any other PR or main: (https://github.com/Unstructured-IO/unstructured/actions/runs/6313633584/job/17143422610?pr=1524) 22min. Not exactly a 2x improvement but something.

oh saw that, my questions was relative to the trade off on runner availability per Ahmet's comment.

@rbiseck3
Copy link
Copy Markdown
Contributor Author

The only reason there was any wait was because Trevor took down the only large instance we had running, now that one is back up there shouldn't be any wait time.

@ryannikolaidis
Copy link
Copy Markdown
Contributor

The only reason there was any wait was because Trevor took down the only large instance we had running, now that one is back up there shouldn't be any wait time.

ahhh perfect

Copy link
Copy Markdown
Contributor

@ryannikolaidis ryannikolaidis left a comment

Choose a reason for hiding this comment

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

looks good, thanks for pushing on this!

@badGarnet badGarnet merged commit 81af879 into main Sep 26, 2023
@badGarnet badGarnet deleted the roman/parallel-ingest-jobs branch September 26, 2023 21:06
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.

4 participants