TorchSharp icon indicating copy to clipboard operation
TorchSharp copied to clipboard

Fix #1356

Open LittleLittleCloud opened this issue 1 year ago • 10 comments

fix #1356

LittleLittleCloud avatar Jul 16 '24 20:07 LittleLittleCloud

What do the CompatibilitySuppressions files do?

It's like suppress warning.

LittleLittleCloud avatar Jul 16 '24 20:07 LittleLittleCloud

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.

yueyinqiu avatar Jul 17 '24 03:07 yueyinqiu

@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

LittleLittleCloud avatar Jul 17 '24 18:07 LittleLittleCloud

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.

yueyinqiu avatar Jul 18 '24 16:07 yueyinqiu

@LittleLittleCloud -- is this PR still relevant?

NiklasGustafsson avatar Jul 25 '24 15:07 NiklasGustafsson

@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

LittleLittleCloud avatar Jul 25 '24 20:07 LittleLittleCloud

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

NiklasGustafsson avatar Jul 26 '24 00:07 NiklasGustafsson

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 avatar Jul 29 '24 11:07 yueyinqiu

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 avatar Sep 27 '24 01:09 yueyinqiu

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 avatar Nov 15 '24 06:11 yueyinqiu