Migrate to jsii-srcmak #276
Conversation
|
It's also extremely slow, so hopefully that's related to the failure |
|
I believe the error I'm encountering is the same as cdk8s-team/cdk8s#273 since both the aws provider and the vpc module are getting set to the |
|
@eladb Before I start profiling things, do you have any idea why jsii-srcmak would be so much slower than jsii + jsii-pacmak in a simple wrapper script? |
No idea. I haven't noticed this myself but now I am curious. Let me know what you find out! |
Yes, for Python it feels pretty slow indeed.
Looks like there might be a fix on master. But the build failed for some reason and therefore no package was published. Perhaps we can give it a shot straight from github? |
Well, without measuring it - the current version feels slow as well. :) |
Thanks! |
| const manifest = JSON.parse(await fs.readFile(jsiiPath, 'utf-8')); | ||
|
|
||
| // patch cdktf version in manifest because it's not stable | ||
| manifest.dependencies.cdktf = '999.999.999'; |
There was a problem hiding this comment.
Could you elaborate on this number?
There was a problem hiding this comment.
It could be anything. That value is just what cdk8s uses.
The idea is that the dependency version doesn't provide any meaningful information to the snapshot and so should be effectively ignored.
|
In my testing, the python integration test went from 4.5 minutes to 27 minutes with this change. I don't think we should merge until that time is cut down. I haven't definitively tracked down the cause yet. My current hunch is that this line: https://github.com/aws/jsii-srcmak/blob/b3ca41e005af735adef79422033d179192875dfc/src/srcmak.ts#L20 is causing a lot of unnecessary file io. |
Ok, that's a huge difference. I'll try this on my machine again. Which kind of setup do you have? Linux, Mac, Windows? |
That's running the same docker container used by the build workflow using docker desktop wsl2 backend on a wsl2 filesystem from Windows. The github runner does also show a decent increase in time as well: going from ~7 minutes for all integration tests to ~23 minutes. |
Sounds like this is something we should be able to fix. Can you guys raise an issue towards jsii-srcmak and we will tackle in short order? |
I'm experementing with #98 and a full jsii build takes 9 minutes. That's with
Also, we probably should upgrade and align all |
While there is some unnecessary work being done (already being called from a temporary directory), this doesn't appear to be the issue. I'm still working to track down the main cause, but once I have more conclusive information I'll make sure to file anything. |
|
Copying @RomainMuller |
|
I suggest you give |
|
@jsteinich thanks for creating the issue in the |
|
is my assumption correct, that this is mostly a CI issue around integration tests? I did some local testing again, and the times for If so, I'd probably ignore the performance regression for now. If it's an actual user facing issue, we should submit a PR to fix aws/jsii#1856 Thoughts? |
The other place where it will show up is the first time someone sets up a python project, or every project init if they use a different virtual environment per project. |
So, a |
If it's the first time running on a system / virtual env, then yes. Subsequent runs use cached data and are just subject to the standard slowness. |
Do you know how to clear that cache to reproduce easily? |
Easiest repro I know is creating a new pipenv. |
Hm, I believe I did that, and in a Docker container I'd get a new one anyway, I guess. |
Doesn't look like it would be too difficult to create a PR to make I'm not sure the urgency of better Windows support, Java support, and anything else that this is holding up though. If any of those are wanted for the next release, I think it would make sense to get this merged in, but then hold on releasing until performance was improved. |
|
We can short circuit Once merge it will be immediately released. |
It would be good to get this merged, so we don't have too many parallel tracks here.
Also a good idea 👍 |
Would you guys like to pick this up? |
I'd feel comfortable stubbing |
|
On it |
|
Okay, coming up: aws/jsii#1864 (by @RomainMuller) |
|
@eladb Any idea when the next jsii release will be? |
Just got released a few hours ago https://github.com/aws/jsii/releases/tag/v1.11.0 |
|
This is likely relevant: #328 |
Looks like jsii-srcmak doesn't pass tests when upgrading to jsii 1.11.0. Once that's fixed and released (let me know if I can help), then I'll get this updated. |
Released jsii-srcmak with jsii 1.11.0 |
|
Awesome work @jsteinich and thanks for the quick support @eladb @RomainMuller |
|
@skorfmann can we close #272? |
|
I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Address #272
Python integration test is currently failing, but all other tests are passing.