Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manual cleanup of workspace during garbage collection #656

Merged
merged 2 commits into from Apr 10, 2024

Conversation

erick-xanadu
Copy link
Contributor

@erick-xanadu erick-xanadu commented Apr 10, 2024

Context: There is a ResourceWarning message with contents similar to the following:

Implicitly cleaning up <TemporaryDirectory '/private/var/folders/91/0sd1rhg56rs8y299zcsyg1hh0000zc/T/f1h7aj47n_'>

This is expected from the TemporaryDirectory, which implicitly cleans up after itself.

It will be destroyed as soon as it is closed (including an implicit close when the object is garbage collected)

When this warning is paired with tests that will treat all warnings as errors, such as when using the decorator:

    @pytest.mark.filterwarnings("error")

All warnings will instead raise an exception:

One can also cause all warnings to be exceptions by using error instead of always.

Which will then make pytest raise another warning

frontend/test/pytest/test_autograph.py::TestForLoops::test_ignore_warnings - pytest.PytestUnraisableExceptionWarning:

which since it is treated as an error, may make a test fail.

Description of the Change: In order to avoid this warning, explicitly clean up. This will remove the initial warning that triggers these events.

Benefits: Avoid a warning / tests to fail.

Drawbacks: It is a bit redundant to cleanup explicitly during garbage collection of the parent object.

@erick-xanadu erick-xanadu marked this pull request as ready for review April 10, 2024 12:44
Copy link

codecov bot commented Apr 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.41%. Comparing base (2b1fde5) to head (0717750).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #656   +/-   ##
=======================================
  Coverage   99.41%   99.41%           
=======================================
  Files          55       55           
  Lines        9121     9124    +3     
  Branches      701      702    +1     
=======================================
+ Hits         9068     9071    +3     
  Misses         28       28           
  Partials       25       25           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@erick-xanadu
Copy link
Contributor Author

erick-xanadu commented Apr 10, 2024

I'm not sure when -p no:warnings was added to the frontend tests, but one can see in the kokkos that that warning is no longer. https://github.com/PennyLaneAI/catalyst/actions/runs/8631468232/job/23660666861?pr=656

One can compare it with the main branch, which does have that warning: https://github.com/PennyLaneAI/catalyst/actions/runs/8619522185/job/23624994862

Copy link
Collaborator

@dime10 dime10 left a comment

Choose a reason for hiding this comment

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

Thank you for figuring this out 💯

@erick-xanadu erick-xanadu merged commit a3562a4 into main Apr 10, 2024
38 checks passed
@erick-xanadu erick-xanadu deleted the eochoa/2024-04-10/manual-cleanup branch April 10, 2024 14:01
erick-xanadu added a commit that referenced this pull request Apr 24, 2024
erick-xanadu added a commit that referenced this pull request Apr 24, 2024
dime10 added a commit that referenced this pull request Apr 26, 2024
**Context:** PR #656 attempted to remove a warning, but experiments
shown that it didn't actually remove the warning.

**Description of the Change:** Revert PR #656. Create derived class from
`tempfile.Tempdir` and remove the warning generated in the `_cleanup`
method.

**Benefits:** No warning.

**Note**: Needs wheels to test different python versions. I will test
once with wheels and then after success I will remove the
ci:build-wheels tag. No-cover needs to be added due to different python
versions.

---------

Co-authored-by: David Ittah <dime10@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants