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

Reduce size of typescript input/output definitions files #8613

Open
Tracked by #11598
XBeg9 opened this issue May 19, 2021 · 15 comments · Fixed by #10831
Open
Tracked by #11598

Reduce size of typescript input/output definitions files #8613

XBeg9 opened this issue May 19, 2021 · 15 comments · Fixed by #10831
Labels
area/codegen SDK-gen, program-gen, convert impact/performance Something is slower than expected kind/enhancement Improvements or new features language/javascript resolution/fixed This issue was fixed size/M Estimated effort to complete (up to 5 days).

Comments

@XBeg9
Copy link

XBeg9 commented May 19, 2021

Team, looks like typescript definitions embedded in one single package (azure-native) drains tons of resources on the development machine, is it possible to divide the package into multi-package / plugin per namespace? Something like: @pulumi/azure-native-network . Another possible solution would be to not create one giant input types definition file and make it per namespace. Thanks

azure-native 2021-05-19 07-37-34

@vtereshyn
Copy link

any updates here?

@lukehoban lukehoban transferred this issue from pulumi/pulumi Aug 18, 2021
@lukehoban
Copy link
Member

lukehoban commented Aug 18, 2021

Moving this one over to pulumi-azure-native, as this issue is largely specific to use of that package which is particularly large.

@lukehoban
Copy link
Member

Cross referencing to #7653 which is related to this - and would address some of the performance impacts related to this.

@XBeg9 XBeg9 changed the title Pulumi generates giant (40M+) input/output typescript definitions files Pulumi generates giant (40MB+) input/output typescript definitions files Aug 19, 2021
@mikhailshilkov mikhailshilkov transferred this issue from pulumi/pulumi-azure-native Dec 20, 2021
@mikhailshilkov
Copy link
Member

I moved this issue to pulumi/pulumi as that's where the TypeScript SDK generation is implemented

@mikhailshilkov mikhailshilkov added area/codegen SDK-gen, program-gen, convert impact/performance Something is slower than expected kind/enhancement Improvements or new features language/javascript labels Dec 20, 2021
@danielrbradley danielrbradley changed the title Pulumi generates giant (40MB+) input/output typescript definitions files Reduce size of typescript input/output definitions files Aug 12, 2022
@danielrbradley
Copy link
Member

This issue is mentioned in pulumi/pulumi-azure-native#932 which is currently the second highest up-voted issue for azure-native.

@t0yv0 t0yv0 self-assigned this Sep 2, 2022
@t0yv0 t0yv0 added this to the 0.78 milestone Sep 2, 2022
@t0yv0 t0yv0 added the size/M Estimated effort to complete (up to 5 days). label Sep 2, 2022
@RobbieMcKinstry
Copy link
Contributor

RobbieMcKinstry commented Sep 15, 2022

Just to explicitly provide an update for the community members following along, this work is slated for implementation.
At first blush, it seems likely we'll be able to build a types file for each resource, but I'm unsure how we can keep the types defined at each index.d.ts file from ballooning memory on load.

@XBeg9
Copy link
Author

XBeg9 commented Sep 15, 2022

@RobbieMcKinstry I don't want to repeat myself again, but there is still an option to divide azure-native into smaller packages like @pulumi/azure-native-network, @pulumi/azure-native-containerregistry and etc, it's very rare that you need everything at once in one single repo. This could be easily published using monorepo like https://github.com/lerna/lerna, you just need to change the way you generate the SDK, exactly what @mikhailshilkov said.

@RobbieMcKinstry
Copy link
Contributor

Thanks for reiterating, @XBeg9. I think you're definitely on to something! Personally, I have to imagine that's a change we'd want to make in the long run, but I'm not on a team with ownership of Azure Native, so my say doesn't matters little compared to the experts :) You're right; virtually nobody needs more than a smattering of resources offered by each cloud provider, and it's odd to make all users pay the same price in terms of memory/disk.

Specifically for Azure Native, I see that the issue of splitting packages is already tracked here. It looks like @danielrbradley has already started on some blocking groundwork here. There's also precedent for doing a similar cleaving of the Go SDK that Daniel recently landed. Not what we're discussing here but in the same ballpark.

My understanding was the Platform team is using this issue to track the splitting of nodes_modules/types/output.d.ts and similar files into individual type declaration files for each namespace. We suspect that if we can split the type definitions, we can improve performance across all cloud providers, even without package splitting.

@RobbieMcKinstry RobbieMcKinstry modified the milestones: 0.78, 0.79 Oct 4, 2022
@RobbieMcKinstry RobbieMcKinstry modified the milestones: 0.79, 0.80 Oct 24, 2022
bors bot added a commit that referenced this issue Nov 11, 2022
10831: Reduce types output r=RobbieMcKinstry a=RobbieMcKinstry

<!--- 
Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation.
-->

# Description

This is a draft PR for #8613.

<!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. -->

Fixes #8613 

## Checklist

<!--- Please provide details if the checkbox below is to be left unchecked. -->
- [x] I have added tests that prove my fix is effective or that my feature works
<!--- 
User-facing changes require a CHANGELOG entry.
-->
- [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the Pulumi Service,
then the service should honor older versions of the CLI where this change would not exist.
You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version
  <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. -->


Co-authored-by: Robbie McKinstry <robbie@pulumi.com>
Co-authored-by: Robbie McKinstry <thesnowmancometh@gmail.com>
bors bot added a commit that referenced this issue Nov 11, 2022
10831: Reduce types output r=RobbieMcKinstry a=RobbieMcKinstry

<!--- 
Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation.
-->

# Description

This is a draft PR for #8613.

<!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. -->

Fixes #8613 

## Checklist

<!--- Please provide details if the checkbox below is to be left unchecked. -->
- [x] I have added tests that prove my fix is effective or that my feature works
<!--- 
User-facing changes require a CHANGELOG entry.
-->
- [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the Pulumi Service,
then the service should honor older versions of the CLI where this change would not exist.
You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version
  <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. -->


Co-authored-by: Robbie McKinstry <robbie@pulumi.com>
Co-authored-by: Robbie McKinstry <thesnowmancometh@gmail.com>
@bors bors bot closed this as completed in 184903a Nov 11, 2022
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Nov 11, 2022
@RobbieMcKinstry
Copy link
Contributor

👋🏻 Hello! I wanted to follow up with some additional information about the change that landed which closed this issue.
In #10831 we changed code generation to output many smaller files instead of a few massive ones.

Background

As of v3.46.1 (the current release at the time of writing), types for each provider are written to types/input.ts or types/output.ts for input types and output types, respectively. Each of those files have namespaces corresponding to the package's modules. This means that if you want the input types for aws/s3, you need to open up types/input.ts, read in the entire file, and access the namespace aws.s3.

Filesystem Diagram, Before

This Change

This will change when #10831 is released (next week, barring regressions). Now, each top-level namespace previously in input.ts corresponds to a subdirectory in types. Those subdirectories will also contain input.ts and output.ts files, which describe just the input/output types declared at that level of granularity, and make re-export types in deeper subfolders.

Filesystem Diagram, After

This is all backward-compatible with the current API. And, I believe, it does not currently offer a performance benefit. All of our input.ts and output.ts files (and resource files for that matter) still import the top-level types/input.ts. So even if you changed your import to import { Foo } from "aws/types/s3/input", which is a smaller file, you won't see a performance benefit, because that input file will still import aws/types/{input, output}.ts which imports everything.

So, I don't expect this change to improve performance. While I haven't run any benchmarks on it yet, we will be monitoring the performance going forward, including before release.

Next Steps

It sucks that this change shouldn't improve performance. ☹️ However, fixing this issue was a blocker for #10442 which will improve performance in exactly the ways we're discussing here. #10442 proposes making our compiler smart enough to localize imports. Let's look at some code:

This is taken from https://github.com/pulumi/pulumi-aws:/ec2/instance.ts

import * as inputs from "../types/input";
// ...
export interface InstanceState {
    // ...
    capacityReservationSpecification?: pulumi.Input<inputs.ec2.InstanceCapacityReservationSpecification>;
    // ...
}

In this snippet, we see that our compiler generates code that imports all input types by pulling in the types/input. Since our API is backward-compatible after #10831 , this file re-exports all of the input files beneath it in the directory tree. Then, when we reference inputs.ec2.InstanceCapacityReservationSpecification, we're dereferencing the types/input.ts module, which dereferences the ec2 namespace, which dereferences InstanceCapacityReservationSpecification.

The primary change that #10442 will introduce which will provide a major performance win is that it will change the code to look like this:

import { InstanceCapacityReservationSpecification } from "../types/input/ec2";
export interface InstanceState {
    capacityReservationSpecification?: pulumi.Input<InstanceCapacityReservationSpecification>;
}

(This isn't the exact syntax, it's still WIP).

This lets us skip importing the entire list of types from aws and go straight to ec2 (or maybe even ec2/input.ts).

However, implementing this is no small feat. It requires adding a compiler pass to identify which imports are required for each namespace. And the blast radius isn't limited to the types directory -- as demonstrated above this optimization will apply to resource definitions too.

I can't make any promises, but I'd like to break ground on #10442 by the end of the month. If I miss that window, however, it's possible the work won't be scheduled since we've budgeted this quarter for Performance Improvements, and it's unclear if there will be future time allocation Q1 2023 for this work.

@RobbieMcKinstry
Copy link
Contributor

I regret to say, we have to reopen this issue. The PR which closed this issue has been reverted due to a bug in TSC. I'm tracking the work needed to get this back into place in a new issue which I'll link below in a few minutes.

@RobbieMcKinstry
Copy link
Contributor

Here's the tracking issue for the related changes.

@vtereshyn
Copy link

Hi! Do you happen to have any updates on this?

@RobbieMcKinstry
Copy link
Contributor

Hi @vtereshyn after backing out my previous PR, we realized we could improve the API when making this change, so we tacked on some extra work to coincide with a repaired version of the original PR. That's all linked in the tracking issue.
When Q1 2023 started, I expressed to my manager that I felt I needed to park this issue for a time (burning out looking at the same code for so long); now that we're entering Q2 I feel better about tackling this issue now. I'm hoping we'll have some bandwidth to schedule this for Q2. The majority of the work is in three steps:

  1. Rebasing the reverted PR to get caught back up with master.
  2. Adding a toggle to allow providers to incrementally enable this feature.
  3. Moving the split files from /types to be colocated with their resources. This is definitely the biggest component.

@vtereshyn
Copy link

@RobbieMcKinstry – thank you for the update.

I feel this will be a huge step forward and a significant improvement. I am looking forward to seeing that in Q2.

@RobbieMcKinstry
Copy link
Contributor

My pleasure! Always happy to chat!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/codegen SDK-gen, program-gen, convert impact/performance Something is slower than expected kind/enhancement Improvements or new features language/javascript resolution/fixed This issue was fixed size/M Estimated effort to complete (up to 5 days).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants