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

fix: modules not built sequentially #3045

Merged
merged 2 commits into from
Oct 8, 2021
Merged

Conversation

njlynch
Copy link
Contributor

@njlynch njlynch commented Oct 7, 2021

For Go, JS, and Python, the OneByOneBuilder is used to build the modules
sequentially. The modules are first sorted topologically, then passed into the
builder to build one at a time. However, the package builds are kicked off
asynchronously, leading to parallel builds and potential build issues when a
local package depends on another.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For Go, JS, and Python, the `OneByOneBuilder` is used to build the modules
sequentially. The modules are first sorted topologically, then passed into the
builder to build one at a time. However, the package builds are kicked off
asynchronously, leading to parallel builds and potential build issues when a
local package depends on another.
@njlynch njlynch self-assigned this Oct 7, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Oct 7, 2021
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment

Choose a reason for hiding this comment

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

I guess we could do a reduce to build a single promise chain here but the for/of loop feels easier to read and fits better with our code style, lint ignores and all. Alternative would be something like:

await this.modules.reduce((previous, mod) => {
  return previous.then(() => this.options.codeOnly
    ? this.generateModuleCode(module, this.options)
    : this.buildModule(module, this.options)
  );
}, Promise.resolve())

@otaviomacedo
Copy link
Contributor

We could have the best of both worlds: chain dependent tasks together and run independent ones in parallel. But of course, there is more effort involved in making this work and may not be worth it.

njlynch added a commit to aws/aws-cdk that referenced this pull request Oct 8, 2021
jsii-rosetta has a bug where modules are packed in parallel, rather than
sequentially. This causes packing failures on v2, where the alpha modules depend
on aws-cdk-lib, but may be packed first.

The jsii fix is aws/jsii#3045. However, to shortcut the
build+test+upgrade+merge+deploy cycle, creating this patch to speed up
verification.
@njlynch njlynch merged commit 1589af8 into main Oct 8, 2021
@njlynch njlynch deleted the njlynch/pacmak-dependencies branch October 8, 2021 09:47
mergify bot pushed a commit to aws/aws-cdk that referenced this pull request Oct 8, 2021
jsii-pacmak has a bug where modules are packed in parallel, rather than
sequentially. This causes packing failures on v2, where the alpha modules depend
on aws-cdk-lib, but may be packed first.

The jsii fix is aws/jsii#3045. However, to shortcut the
build+test+upgrade+merge+deploy cycle, creating this patch to speed up
verification.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
njlynch added a commit to aws/aws-cdk that referenced this pull request Oct 8, 2021
The pack on v2 is currently broken due to a bug in jsii-pacmak <= 1.38.0 that
packs all modules in parallel, regardless of dependencies between the
modules. That breaks on building the alpha modules, since they rely on
aws-cdk-lib, but often end up being packed first. A fix was created in jsii
(aws/jsii#3045), but to unblock the build earlier, a
patch was created to apply the fix within the CDK prior to the next jsii release
(#16871).  Unfortunately, our build
"transform" step runs a `lerna` command that re-installs jsii-pacmak, removing
the patch. Calling `yarn install` here again to re-install and re-apply the
patch prior to packing.
njlynch added a commit to aws/aws-cdk that referenced this pull request Oct 8, 2021
The pack on v2 is currently broken due to a bug in jsii-pacmak <= 1.38.0 that
packs all modules in parallel, regardless of dependencies between the
modules. That breaks on building the alpha modules, since they rely on
aws-cdk-lib, but often end up being packed first. A fix was created in jsii
(aws/jsii#3045), but to unblock the build earlier, a
patch was created to apply the fix within the CDK prior to the next jsii release
(#16871).  Unfortunately, our build
"transform" step runs a `lerna` command that re-installs jsii-pacmak, removing
the patch. Calling `yarn install` here again to re-install and re-apply the
patch prior to packing.
mergify bot pushed a commit to aws/aws-cdk that referenced this pull request Oct 11, 2021
The pack on v2 is currently broken due to a bug in jsii-pacmak <= 1.38.0 that
packs all modules in parallel, regardless of dependencies between the
modules. That breaks on building the alpha modules, since they rely on
aws-cdk-lib, but often end up being packed first. A fix was created in jsii
(aws/jsii#3045), but to unblock the build earlier, a
patch was created to apply the fix within the CDK prior to the next jsii release
(#16871).  Unfortunately, our build
"transform" step runs a `lerna` command that re-installs jsii-pacmak, removing
the patch. Calling `yarn install` here again to re-install and re-apply the
patch prior to packing.

_Note: This will become obsolete once jsii-pacmak 1.39.0 is released_

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
rix0rrr added a commit that referenced this pull request Nov 5, 2021
In #3045, the `OneByOneBuilder` was switched to build all packages
in series (instead of all in parallel), because there would
be race conditions in case of inter-package dependencies.

This is correct, but leaves a bunch of possible parallelism on the
table that is notably blowing up pack times for Python.

Re-introduce a (limited) form of parallelism by retaining the
sets of mutually independent packages, as toposorted, and
doing those in parallel.
mergify bot pushed a commit that referenced this pull request Nov 8, 2021
In #3045, the `OneByOneBuilder` was switched to build all packages
in series (instead of all in parallel), because there would
be race conditions in case of inter-package dependencies.

This is correct, but leaves a bunch of possible parallelism on the
table that is notably blowing up pack times for Python.

Re-introduce a (limited) form of parallelism by retaining the
sets of mutually independent packages, as toposorted, and
doing those in parallel.

Rename the class to `IndependentPackageBuilder` to more clearly
describe the intent behind the class.


---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants