🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/171275

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit the bot commands wiki

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 1978158 with merge base 3d9ce8d ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

TBH, I'm not sure this test is gonna help that much for prevention. It just adds friction any time someone adds a new config option. They'll see a test failure, investigate, and very likely just run with EXPECTTEST_ACCEPT=1 without thinking through the implications (because they won't have the context). What we really want is to catch sources of non-determinism. In practice, those have always come from paths that vary among hosts. It would be nice if we could execute the test across machines and compare, but I doubt we can do that easily (either in OSS or internally). As an alternative, how would you feel about scanning the config options and just looking (heuristically) for any that look like a path or have the current machine or username somehow embedded in the config value?

TBH, I'm not sure this test is gonna help that much for prevention. It just adds friction any time someone adds a new config option. They'll see a test failure, investigate, and very likely just run with EXPECTTEST_ACCEPT=1 without thinking through the implications (because they won't have the context). What we really want is to catch sources of non-determinism. In practice, those have always come from paths that vary among hosts. It would be nice if we could execute the test across machines and compare, but I doubt we can do that easily (either in OSS or internally). As an alternative, how would you feel about scanning the config options and just looking (heuristically) for any that look like a path or have the current machine or username somehow embedded in the config value?

@masnesral I tried to implement test with multi hosts. It's not clear whether it's justified to such a test with high complexity and questionable maintainability in the CI in the long term.

I agree it's a friction. We could implement other detection mechanisms for nondeterminism with much higher cost. This is a compromise to surface the changes made to config in the short term. Open to ideas if this could be addressed systematically in the long term.

I'm not sure whether dir path detection solves the problem of non-determinism in general. If something is dependent on the environment, it shouldn't matter if it's a dir path or not. In fact if we look at ignored factors here https://www.internalfb.com/code/fbsource/fbcode/caffe2/torch/_inductor/config.py only 1 out of 15 configs ignored there is a path, therefore I think the chance to directly address this SEV is very small if we only look at directory.

If the major concern is about friction, we can roll this out only with string fields and see whether it works.

@zhxchen17 has imported this pull request. If you are a Meta employee, you can view this in D90331815.

@pytorchbot merge

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Merge failed

Reason: New commits were pushed while merging. Please rerun the merge command.

If the major concern is about friction, we can roll this out only with string fields and see whether it works.

No I'm mostly worried that we haven't actually solved the problem. It's possible we've made it worse: if someone "fixes" a future failing test by adding to the blocklist when they shouldn't then we risk serving illegal cache entries. I agree that detecting non-determinism across hosts is hard, so I wasn't advocating that. I was arguing it would be worthwhile to at least target a few patterns that we have experience as having been problematic: usernames in config options, hostnames in config options, paths in config names (specifically paths that look like they come from unpacking a .xar file).

if someone "fixes" a future failing test by adding to the blocklist

Yea this sounds a reasonable failure mode for this test.

We have experience as having been problematic: usernames in config options, hostnames in config options, paths in config names (specifically paths that look like they come from unpacking a .xar file).

Updated the PR to do this instead.

This looks nice. Thanks!

@pytorchbot merge

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team