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 #1356 #1357

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix #1356 #1357

wants to merge 5 commits into from

Conversation

LittleLittleCloud
Copy link
Contributor

fix #1356

@LittleLittleCloud LittleLittleCloud marked this pull request as ready for review July 16, 2024 20:06
Copy link
Contributor

@NiklasGustafsson NiklasGustafsson left a comment

Choose a reason for hiding this comment

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

What do the CompatibilitySuppressions files do?

@LittleLittleCloud
Copy link
Contributor Author

What do the CompatibilitySuppressions files do?

It's like suppress warning.

@yueyinqiu
Copy link
Contributor

yueyinqiu commented Jul 17, 2024

It looks like a better approach would be to create the file automatically when packing? since there would still be new stuffs added into TorchSharp in the future, and in the current stage we don't that mind API changes.

@LittleLittleCloud
Copy link
Contributor Author

@yueyinqiu

Yeah that would be helpful if the suppression file can be generated during packing so we can check in the supression file if necessary as well as detecting the possible unintentional API break change

@yueyinqiu
Copy link
Contributor

Yes but that means we have to separate the pack stage and publish stage? Actually I mean it's a bit early to check for the API compatibility, so we could just generate the suppression file and get into the next step.

@NiklasGustafsson
Copy link
Contributor

@LittleLittleCloud -- is this PR still relevant?

@LittleLittleCloud
Copy link
Contributor Author

@NiklasGustafsson Yes, I'm thinking of uploading the suppression file to pipeline artifacts so we no longer need to generate it locally. After the suppression file getting uploaded, the workflow for processing API break change will be

  • in PR check: the ci fails if the PR includes API break change and upload new suppression file to pipeline artifacts
  • if the API break change is on purpose, update the suppression file in PR and fix the validation error
  • if the API break change is unexpected, fix the API break change in PR

@NiklasGustafsson
Copy link
Contributor

So we won't have to wait until after merge to find out about the breakage?

@yueyinqiu
Copy link
Contributor

I think a better expression would be we 'shouldn't'. We should know whether a PR will break the API before merging it.

If we only check that before packing and publishing, it's almost impossible to find the specific commit which changed the API and revent it.

@yueyinqiu
Copy link
Contributor

yueyinqiu commented Sep 27, 2024

Hmm... are there any news about this? I'm afraid of new commits since they may break APIs which can't even be find out until packing.

By the way, will the fix in #1376 fail due to this as well?

@yueyinqiu
Copy link
Contributor

yueyinqiu commented Nov 15, 2024

does #1409 and #1410 solve the problem?

I've also found a way to do this without changing the current api. But that leads to two (almost) independent dataset/dataloader system, which might be confusing.

@yueyinqiu yueyinqiu mentioned this pull request Nov 15, 2024
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.

Suppress the API breakchange introduced by PR 1354 and bump up minor version
3 participants