temporary fix for jit test backward compatibility issues

[albanD]

Stack from ghstack:

• temporary fix for jit test backward compatibility issues #31949 temporary fix for jit test backward compatibility issues

Differential Revision: D19314763

[kostmo]

💊 CircleCI build failures summary and remediations

As of commit dcd22a9:

Commit dcd22a9 was recently pushed. Waiting for builds...

---

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

This comment has been revised 1 time.

[houseroad]

The test is still broken

TypeError: append() takes exactly one argument (2 given)

So create a tuple? :-)

[albanD]

Yes I was doing something else, fixed now.

[houseroad]

Nit: Also, just use _TorchScriptTesting_ should be enough, and you can directly add this to the whitelist, and add a comment at the end.

[albanD]

I wanted to be as restrictive as possible to discourage people from adding more until we get a proper solution for the problem. But I dont have a very strong opinion. You think the more generic check is better?

[houseroad]

If so, we should add comments around jit_test_functions to warning the developers in future, and tell them don't add new testing functions. They will see this note when they try to add new items to white list in future.

[houseroad]

Unblock, please merge after the BC CI is green :-)

[facebook-github-bot]

@albanD merged this pull request in c299cb0.

[bhosmer]

@albanD @houseroad sorry for the late comment, but - since these are exactly the signatures that are deliberately omitted when loading the new schemas from file (here), I'm curious why we wouldn't just explicitly factor the if "__torch__.torch.classes" in filter out into a helper and call it from both places?

No idea the history here, so apologies if the question is nonsense :)

[albanD]

@bhosmer I am not sure. The nice thing with the whitelist is that we can give a timeout to make sure the issue get solved.

[bhosmer]

@albanD yeah that's nice. I just hope the connection with the TODO logic in the new-schema file loader doesn't get lost. @houseroad that was a recent addition, right? Does that TODO have a definite future or do we risk having to rediscover the connection when the whitelist times these additions out?

[houseroad]

#31982 will remove if "__torch__.torch.classes" in, which was only for a temporary fix. The whitelist works after we load all the new functions. However, sometimes, we get problem to load schemas, if "__torch__.torch.classes" in is to bypass the problem.

[bhosmer]

@houseroad ah got it, thanks for the info!