🔗 Helpful Links

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

• 📄 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 or our office hours

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

❌ 2 New Failures, 1 Unrelated Failure

As of commit 0663f08 with merge base ecea811 ():

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

@H-Huang @awgu @wanchaol @fegin @fduwjj @wz337 @wconstab @d4l3k --> This is now ready for review.

It just moves the argparse command line parsing out of the root code of common_utils into a function which needs to be called by the tests from their __main__.

Unfortunately a lot of tests (Mostly the distributed ones) relied on side effects from that code like for example setting the seed in worker processes to the default seed (Note: the seed set on the command line was never passed to the children processes, so it always used the default value).

Looks fine, sanity checked that the number of tests seem to stay the same, not really related but I wonder how necessary it is for some of the globals to actually be globals

Yes, I totally agree: some of the tests (Especially the distributed ones) rely on global states being implicitly set in all workers but I didn't have access to machines to run those tests on GPUs so I decided to play it safe and not refactor them too much 😅

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / linux-jammy-rocm-py3.10 / test (default, 1, 2, linux.rocm.gpu.2)

@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

@pytorchbot revert -m "breaking tests internally with assert common_utils.SEED is not None" -c ghfirst

Breaking lots of tests (included distributed module) internally, with the same issue: "assert common_utils.SEED is not None".

test_create_sharded_tensor_with_ones (test_sharded_tensor.TestShardedTensorChunked)
Test sharded_tensor.ones(...) ... /data/sandcastle/boxes/trunk-hg-full-fbsource/buck-out/v2/gen/fbcode/c0c096af71f133c3/caffe2/test/distributed/_shard/sharded_tensor/__sharded_tensor__/sharded_tensor#link-tree/torch/testing/_internal/common_utils.py:2450: UserWarning: set_rng_seed() was called without providing a seed and the command line arguments haven't been parsed so the seed will be set to 1234. To remove this warning make sure your test is run via run_tests() or parse_cmd_line_args() is called before set_rng_seed() is called.
  warnings.warn(msg)
FAIL

======================================================================
FAIL: test_create_sharded_tensor_with_ones (test_sharded_tensor.TestShardedTensorChunked)
Test sharded_tensor.ones(...)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/sandcastle/boxes/trunk-hg-full-fbsource/buck-out/v2/gen/fbcode/c0c096af71f133c3/caffe2/test/distributed/_shard/sharded_tensor/__sharded_tensor__/sharded_tensor#link-tree/torch/testing/_internal/distributed/_shard/sharded_tensor/__init__.py", line 71, in setUp
    self._spawn_processes()
  File "/data/sandcastle/boxes/trunk-hg-full-fbsource/buck-out/v2/gen/fbcode/c0c096af71f133c3/caffe2/test/distributed/_shard/sharded_tensor/__sharded_tensor__/sharded_tensor#link-tree/torch/testing/_internal/common_distributed.py", line 749, in _spawn_processes
    self._start_processes(proc)
  File "/data/sandcastle/boxes/trunk-hg-full-fbsource/buck-out/v2/gen/fbcode/c0c096af71f133c3/caffe2/test/distributed/_shard/sharded_tensor/__sharded_tensor__/sharded_tensor#link-tree/torch/testing/_internal/common_distributed.py", line 720, in _start_processes
    assert common_utils.SEED is not None
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError

probably parse_cmd_line_args() need to be added to the relevant test suits / frameworks.

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@AnthonyBarbier your PR has been successfully reverted.

@pytorchbot revert -m "breaking tests internally with assert common_utils.SEED is not None" -c ghfirst

Breaking lots of tests (included distributed module) internally, with the same issue: "assert common_utils.SEED is not None".

@izaitsevfb What do you mean "internally" ? What am I supposed to do about it?

@izaitsevfb What do you mean "internally" ? What am I supposed to do about it?

I mean that the issue was spotted when importing the change into Meta's codebase. However, distributed tests are open source, to name a few:

• FAIL: test_unshard_async (fsdp.test_fully_shard_comm.TestFullyShardUnshardMultiProcess)
• FAIL: test_train_mixed_requires_grad_per_group (fsdp.test_fully_shard_frozen.TestFullyShardFrozen)
• FAIL: test_bf16_hook_has_wrapping_False_sharding_strategy2 (test_fsdp_comm_hooks.TestCommunicationHooks)

I'm not sure why this issue wasn't surfaced in OSS (do we run distributed tests?). The fix might be as simple as adding parse_cmd_line_args() here:

I'd suggest to try repro the failure and ask @clee2000 or myself after the fix to re-import this PR again internally to double check that there are no more regressions.

@izaitsevfb @clee2000 Could you please provide more details about the command lines used to run those tests and share the logs? (I don't have access to an environment with multiple GPUs to reproduce the issue and I haven't managed to hit by trying to hack the tests to run on CPU)

The fix might be as simple as adding parse_cmd_line_args() here:

I disagree, the correct sequence should be:

• Declare test suites / test cases
• Parse command line arguments / set globals
• Instantiate tests
• Run tests

The problem this PR is trying to fix is the fact test suites can't be imported into larger test suites because simply importing their module will trigger parsing command line arguments which might be completely irrelevant / different in that new context.

As part of moving the command line parsing to the run_tests() function, it became clear that a large number of tests actually rely on global settings to declare the tests (i.e all the modules which have some kind of x = torch.randn() at their top level will implicitly rely on the seed being set or they won't be reproducible) so as a workaround I allowed some test suites to call parse_cmd_line_args() before declaring their tests. It's dodgy but still a bit better.

Now the distributed tests are a different level of dodgy 😅 : because they'll parse the command line in the parent process, then create a pool of processes which will not have access to the original command line arguments and therefore all the children's global states will implicitly initialise all globals the default values on import common_utils, and a lot of tests rely on this pattern of a whole bunch of globals being implicitly initialised to the same value in all the workers.
Anyway, in this case I did try to fix things by passing the seed from the parent to the children so that the command line options actually now do something.

Traceback (most recent call last):
File "/data/sandcastle/boxes/trunk-hg-full-fbsource/buck-out/v2/gen/fbcode/c0c096af71f133c3/caffe2/test/distributed/_shard/sharded_tensor/sharded_tensor/sharded_tensor#link-tree/torch/testing/_internal/distributed/_shard/sharded_tensor/init.py", line 71, in setUp
self._spawn_processes()
File "/data/sandcastle/boxes/trunk-hg-full-fbsource/buck-out/v2/gen/fbcode/c0c096af71f133c3/caffe2/test/distributed/_shard/sharded_tensor/sharded_tensor/sharded_tensor#link-tree/torch/testing/_internal/common_distributed.py", line 749, in _spawn_processes
self._start_processes(proc)
File "/data/sandcastle/boxes/trunk-hg-full-fbsource/buck-out/v2/gen/fbcode/c0c096af71f133c3/caffe2/test/distributed/_shard/sharded_tensor/sharded_tensor/sharded_tensor#link-tree/torch/testing/_internal/common_distributed.py", line 720, in _start_processes
assert common_utils.SEED is not None
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError

Now, this stacktrace really confuses me because it suggests the process pool is either created in the main process before the command line arguments are parsed or the test runs in a subprocess which itself tries to create a new process pool.

AFAICT this test comes from this file which does have a run_tests() at the bottom? So is there special in the way these tests are instantiated / run?

The internal test runner is pretty different, I don't think it gives command line arguments at all or uses the normal main block, so you might have to provide a default value for SEED and some other vars

The internal test runner is pretty different, I don't think it gives command line arguments at all or uses the normal main block, so you might have to provide a default value for SEED and some other vars

This assert does exactly what it's supposed to do: it catches the fact you're trying to use an uninitialised global variable (Which wouldn't happen if you were not bypassing the main!).

If I remove this assert / set a global default value, we go back to the original issue that there is no way to catch if a test forgets to parse the command line arguments and therefore they'll just get ignored.

At that point we might as well just remove the "--seed" argument to be honest, which I guess is the other option?

If you want to bypass the main in your infrastructure then you need to get your test runner to either call parse_cmd_line_args() once to initialise the globals before you start running things or to manually set the globals: it's probably mostly just SEED and GRAPH_EXECUTOR which are used by tests I think.

Hm is it necessary to parse command line vars? I don't think you're supposed to run distributed tests using pytest directly usually, but it is something that works with most other tests, and pytest wouldn't run the main block either? I don't know a lot about the internal test runner, but I'm pretty sure it uses pytest, but I don't know the details about test running and discovery

Hm is it necessary to parse command line vars? I don't think you're supposed to run distributed tests using pytest directly usually, but it is something that works with most other tests, and pytest wouldn't run the main block either? I don't know a lot about the internal test runner, but I'm pretty sure it uses pytest, but I don't know the details about test running and discovery

I don't think it's something to do with pytest, it's just running the test from the command line in general.

Anyway, if we think it's a torch.distributed thing, then I could hardcode a seed for distributed tests only and print a warning if the user tries to set a different one from the command line indicating it's not supported and will have no effect.

Adding ciflow/periodic label to trigger distributed signals.