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

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

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

andrueastman
Copy link
Member

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
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

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
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
Member 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
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
24 checks passed
@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