Skip to content

Revert "Sets IsComposable=true for workbook* bound types"#590

Merged
andrueastman merged 1 commit intomasterfrom
revert-585-is/set-composable-fxns
Mar 18, 2024
Merged

Revert "Sets IsComposable=true for workbook* bound types"#590
andrueastman merged 1 commit intomasterfrom
revert-585-is/set-composable-fxns

Conversation

@andrueastman
Copy link
Copy Markdown
Contributor

Temporarily Reverts #585

The generation pipeline isn't able to push some yamls because of the size exceeding 100MB so we would need to resolve #184 first to enable git LFS if we are to go forward.
#585 (comment)

cc @baywet @irvinesunday

@andrueastman andrueastman requested a review from a team as a code owner March 13, 2024 06:38
Copy link
Copy Markdown
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

If I'm not mistaken, the before size is around 70MB. This would a 50% + increase. The impact on the SDKs is probably around the same.
Before we revert the change entirely, do we have an idea of what's causing the over expansion?
Customers have been requesting support for Excel endpoints for the longest time, and we're very close to enabling them.
If we have time to do so, I'd prefer waiting to merge that PR and looking into the details.
Maybe there's another contains target is composable we can switch to solve that.
But let's make sure we time box the investigation.

@sebastienlevert
Copy link
Copy Markdown

The size of the resulting SDKs worries me. It's not necessarily related to this change, but if it's something that explodes our SDK size but provide very niche value, I'm proposing we think deeper about our generation logic and identify creative ways to deliver the value in some alternative ways. This might be a GREAT case of self-serve docs?

@irvinesunday
Copy link
Copy Markdown
Contributor

The original doc has a size of appr. 28 MBs, whereas the new one has an appr. size of 238 MBs.
Here's a diff of the paths: https://www.diffchecker.com/FmhW4g35/

The drives API has had an over 300% increase as a result.

@andrueastman
Copy link
Copy Markdown
Contributor Author

@baywet I think we should probably think about going ahead with reverting this PR for now given the size increases.

As an aside we can probably look into the NavigationType annotation which may not necessarily apply to functions/actions.
An alternative is also adding a conversion setting in the library to limit the expansion of paths based on depth and use that to verify the impact (significant paths added look like paths segments get repeated again once a subpath returns the same type as the base)

@baywet
Copy link
Copy Markdown
Member

baywet commented Mar 18, 2024

@andrueastman sure, let's go ahead with a revert to unblock weekly generations.
@irvinesunday can you please also reopen the original issue and post the findings there?
@sebastienlevert self-serve won't work since reverting this means we won't have the endpoints in the OAI description.

@andrueastman andrueastman merged commit 9e05906 into master Mar 18, 2024
@andrueastman andrueastman deleted the revert-585-is/set-composable-fxns branch March 18, 2024 13:33
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.

evaluate enabling git lfs on this repo

4 participants