Skip to content

Handle etxtbsy error in python sdk.#3905

Merged
grouville merged 2 commits into
dagger:mainfrom
sipsma:py-text-busy
Nov 18, 2022
Merged

Handle etxtbsy error in python sdk.#3905
grouville merged 2 commits into
dagger:mainfrom
sipsma:py-text-busy

Conversation

@sipsma
Copy link
Copy Markdown
Contributor

@sipsma sipsma commented Nov 17, 2022

Signed-off-by: Erik Sipsma erik@sipsma.dev

More background on this case in the previous PR here (and in the comment included in the code of this PR): #3763

Apparently despite our expectations that being sync would stop this, @grouville hit it, so may as well just handle the error the same as we do in the Go SDK.

@sipsma sipsma requested a review from helderco November 17, 2022 20:25
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@helderco helderco left a comment

Choose a reason for hiding this comment

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

Error messages are not consistent with the following ones, but I can take care of that later.

Took 5 mins in a VPS with 1 CPU to run this test 🙂 I saw it reach a load of ~30 which matches the concurrency amount in the test.

@grouville
Copy link
Copy Markdown
Member

Error messages are not consistent with the following ones, but I can take care of that later.

Took 5 mins in a VPS with 1 CPU to run this test 🙂 I saw it reach a load of ~30 which matches the concurrency amount in the test.

Thanks to both of you 🙏. In my PR, I'm lowering it to 5 parallel tests (to keep it consistent with the Go SDK). We had to reduce the amount because virtualization on MacOS runners is very slow:

  1. it led to timeouts / inconsistencies
  2. MacOS minutes are 10x more expensive, the amount was arbitrary, and 5 is ok (according to Erik)

@grouville grouville merged commit e364d84 into dagger:main Nov 18, 2022
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.

3 participants