🌍↔️🐍 Limit the number of simultaneous executions

[agahkarakuzu]

Following discussions on #2413 and #1831, this PR introduces --execute-concurrency <n> only.

@agoose77 I tested this locally and it seems to be working. I did not implement an if (execute) { conditional in process::site, which as @fwkoch noted is not really needed. The only subtlety is the difference between the “effective” and “apparent” number of pages to execute, but users are free to set as low as 1 if desired, noted in the docs.

I hope I got the changeset right 👀

[changeset-bot]

🦋 Changeset detected

Latest commit: 60717fc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
myst-cli	Patch
mystmd	Patch
myst-migrate	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[bsipocz]

Thank you so much for fixing this. I have only have a few minor comments.

[agoose77]

@agahkarakuzu I think I'd like to change the approach here to use a semaphore, rather than limiting transforms of the pages. I am thinking of limiting execution as a particular example of concurrency control that differs from the general concurrent processing problem. A reasonable amount of transform work is network requests, so I'd like for those to remain in-flight where possible — and we probably want another setting for throttling concurrent fetch in future.

I can do the work to rework this to use e.g. https://www.npmjs.com/package/async-mutex, or if you have the capacity, feel free. The idea would be to pass the semaphore in to the transformMdast function, and use the runExclusive method to decorate the kernelExecutionTransform — I don't think concurrency is something that transform needs to worry about.

Longer term, we'll end up refactoring this to have some ExecutionOrchestrator handle this, but I think that relates to #2413.

[agahkarakuzu]

@agoose77 that's indeed a cleaner solution and paves the way better for an orchestration approach. I'll give it a stab.

[agoose77]

What a star! ⭐

[agahkarakuzu]

@agoose77 I added semaphore.runExclusive at the kernelExecutionTransform level as you suggested, and it appears to work. In the example below, the first notebook that sleeps for 10 seconds is correctly blocking the rest of the executable content.

I am just not really sure if this is elegant or acceptable:

const opts = { ...program.opts(), ...this.opts() } as SessionOpts;

But I needed a way to pass --execute-parallel to this session.

[choldgraf]

Can you add a short "this is useful when___" type of sentence so users know why they might want to do this?

[choldgraf]

will this ensure that --execute-parallel foo doesn't work? Should we test for that?

[choldgraf]

What happens if --execute-parallel 0?

[choldgraf]

I think this looks useful! Thanks for the contribution. My main suggestion is that we add some kind of testing (both for edge cases like --execute-parallel foo and to ensure it's actually doing what we think it is).

[agahkarakuzu]

Thanks for the feedback @choldgraf, I’ve pushed updates to address your comments.

Regarding testing, we could add three notebooks that each wait for about five seconds, then assert that the total build time is at least fifteen seconds when --execute-parallel 1, and around 5 seconds when --execute-parallel 3?

Since the transformations run in a non-deterministic order, it is not possible to create a test case where the output of one serves as the input to another.

[choldgraf]

hmmm, could we make that more like .5 seconds each? I don't want to just auto-add 15 seconds to the test suite 😅

If nobody has better ideas for how to test this, I'd also be fine just leaving it and seeing if users complain about it or not

[agoose77]

I think the fastest robust way to do this is to use concurrency primitives like barrier and mutex. The easiest thing is probably to create these in a python process, and share their pickled representations via the filesystem or env vars. This would require a small addition to the test harness.

We can then ensure that
A) only a single document can hold a lock at once. If the lock is busy, we fail.
B) all files grab the lock before execution can proceed for any of them. If the barrier times out, we fail.

[agoose77]

Testing this logic is a bit awkward because the implementation is at the end-to-end (E2E) level, rather than within the core execution package.

This makes sense from a design standpoint, because the concept of dependencies and ordering is a higher level one than "execute this file". Unfortunately, to then test this is fiddly because we need to do so in a mostly stateless way.

For now, it's possible to implement some good-enough tests that use the filesystem and delays (although these are sensitive to kernel startup time).

Once we have dependency ordering, it should simplify our tests but we might wish to implement a helper such as a socket server that enforces resource counting (e.g. ensure this lock counter never exceeds 1, or ensure this lock counter reaches 2).

[agoose77]

I've run out of time to debug this further today, so I'd welcome anyone to take it over the line. I'm not immediately sure why the tests pass locally but fail on CI.