test_prototype_datasets_builtin: Properly close all streams

[SvenDS9]

In pytorch/data#1016 I noticed that if a test fails subsequent tests will also fail as not all streams are being closed correctly due to:

for stream in StreamWrapper.session_streams.keys():
RuntimeError: dictionary changed size during iteration

This addresses this issue so that only the "necessary" tests fail.
In theory we should also be able to remove l.135-136 but to be safe I have kept them.

if StreamWrapper.session_streams:
    raise pytest.UsageError(make_msg_and_close("A previous test did not close the following streams:"))

cc @pmeier @bjuncek

[pytorch-bot]

🔗 Helpful Links

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

• 📄 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.

❗ 1 Merge Blocking SEVs

There is 1 active merge blocking SEVs. Please view them below:

• (merge blocking) Issue with internal merge, blocking merges until further notice

If you must merge, use @pytorchbot merge -f.

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

[pmeier]

Thanks @SvenDS9 for the report and fix. The error looks quite weird to me TBH. We are never touching StreamWrapper.session_streams.keys() so why would this happen? Could it be that stream.close() removes it on the fly and thus causing this? If so, wouldn't it be sufficient to just copy the iterator, e.g. for stream in list(StreamWrapper.session_streams.keys())?

[SvenDS9]

Thanks @SvenDS9 for the report and fix. The error looks quite weird to me TBH. We are never touching StreamWrapper.session_streams.keys() so why would this happen? Could it be that stream.close() removes it on the fly and thus causing this? If so, wouldn't it be sufficient to just copy the iterator, e.g. for stream in list(StreamWrapper.session_streams.keys())?

I think that is what happens yes. And you are right, that is sufficient. Will change it now.

[pmeier]

LGTM, thanks @SvenDS9! This will be available in tomorrows nightly release. Could you push an update to pytorch/data#1016 after roughly 12:00 UTC+0 tomorrow to see if this actually fixed your issue?

[github-actions]

Hey @pmeier!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

[SvenDS9]

I have already run the test with my changes to both the test and the datapipe. It unfortunately does not. I would be very grateful if you could take a look why the stream isn't being closed correctly. as you probably have a much deeper understanding what actually happens in the tests... i have linked the relelvant parts in the PR. Please don't feel obligated to do so as I have already wasted enough time on this :D

[pmeier]

I have already run the test with my changes to both the test and the datapipe. It unfortunately does not.

Sorry, I was not clear enough above. Could you make sure that the issue of failing unrelated tests that was addressed by this PR is fixed? Meaning, after this PR you should only see relevant test failures on the TorchData PR, right?

I would be very grateful if you could take a look why the stream isn't being closed correctly. as you probably have a much deeper understanding what actually happens in the tests

I'll look into it if I find some time next week. No promises however.