Skip to content
This repository was archived by the owner on Sep 10, 2025. It is now read-only.

Conversation

@Nayef211
Copy link
Contributor

@Nayef211 Nayef211 commented Aug 29, 2022

Description

  • Currently tests in CI are failing due to running out of disk space (i.e. see example failure)
  • This happens because some of the checkpoints downloaded in the integration test are quite large (around 3GB) and several checkpoints are downloaded for the TF and the RoBERTa models
  • We mitigate this by creating a pytest fixture which creates temporary directories that are deleted once all tests in a test class finish running. This is similar to how torchaudio handles asset deletion in their integration tests (see reference code)
    • The obvious tradeoff is that this will cause more network usage since a single checkpoint may need to be downloaded several times (as several tests may rely on the same checkpoint)
    • We try to mitigate this issue by grouping tests that use the same asset in a single test class

Testing

  • Ensure all CI tests are passing
  • pytest test/prototype/integration_tests/test_models.py
  • pytest test/integration_tests/test_models.py

Followup Items

  • Reorganize tests inside of a torchtext_unittest folder and create a separate workflow to run all integration tests (Fix test execution in torchtext #1889)
  • Remove subclassing from TorchtextTestCase for integration tests to remove any conflicts between unittest.TestCaseand pytext

Copy link
Contributor

@mthrok mthrok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, instead of manually calling conditional_remove, the deletion logic should be automated and the pre-condition should be configured in a way it's independent from rest of the test/library logic. (i.e. it should be turned on/off independent of what ever library/test logics are)

That will avoid the case where future contributors forget to add conditional_remove.

@Nayef211 Nayef211 changed the title Delete model checkpoints within integration test Create pytest fixture to auto delete model checkpoints within integration tests Aug 31, 2022
@Nayef211 Nayef211 mentioned this pull request Aug 31, 2022
2 tasks
@Nayef211 Nayef211 marked this pull request as ready for review August 31, 2022 16:36
@Nayef211 Nayef211 requested a review from mthrok August 31, 2022 16:36
Copy link
Contributor

@mthrok mthrok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stamp

@Nayef211 Nayef211 merged commit 5087134 into pytorch:main Sep 1, 2022
@Nayef211 Nayef211 deleted the test/delete_checkpoints_after_integration_test branch September 1, 2022 13:43
Copy link

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to get torchtext into conda-forge (xref #2023), and currently stumbling over this fixture when trying to run the tests.

Comment on lines +18 to +20
@pytest.fixture(scope="class")
def temp_hub_dir(tmp_path_factory, pytestconfig):
if not pytestconfig.getoption("--use-tmp-hub-dir"):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up in #1889 aside, I don't understand how this can work at all.

Both tmp_path_factory and pytestconfig appear nowhere else in the code base (as far as GH search and git grep go, at least), and based on the pytest docs this should probably be

Suggested change
@pytest.fixture(scope="class")
def temp_hub_dir(tmp_path_factory, pytestconfig):
if not pytestconfig.getoption("--use-tmp-hub-dir"):
@pytest.fixture(scope="class")
def temp_hub_dir(tmp_path_factory, request):
if not request.config.getoption("--use-tmp-hub-dir"):

but that still leaves me mystified as to where tmp_path_factory is coming from.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that still leaves me mystified as to where tmp_path_factory is coming from.

Ah, just found that it's a fixture that comes with pytest by default.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, and so's pytestconfig. In any case, my suspicion now is that this only works when called from within the integration_tests folder, because (I guess) recursive conftest.pys are not evaluated when the CLI is called from a folder that's further out.

Copy link
Contributor Author

@Nayef211 Nayef211 Jan 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@h-vetinari yup I think your understanding is correct. Does only being able to run this test from within the integration_tests folder pose an issue for adding torchtext to conda-forge?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does only being able to run this test from within the integration_tests folder pose an issue for adding torchtext to conda-forge?

No, I just had to realise the reason, after that it's simple. ;)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants