Skip to content

Improved distributed tests #8978

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

Merged

Conversation

arlesniak
Copy link
Contributor

@arlesniak arlesniak commented Feb 27, 2024

This PR fixes distributed tests in cases when an Exception raised in subprocess is not correctly manifested to pytest, finally reported as PASSED, causing false positive result.
Changes made:

  • assert_run_mproc test utility function for decorating and running Processes
  • MPCaptOutput context utility class, capturing std output and stderr

@arlesniak arlesniak force-pushed the arlesniak_intel/pytest_stderr_mp_queue_pr branch 2 times, most recently from cf1e0aa to 3dfab31 Compare February 27, 2024 18:12
@arlesniak arlesniak force-pushed the arlesniak_intel/pytest_stderr_mp_queue_pr branch from 23f6716 to 3ca6a06 Compare March 1, 2024 08:27
Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 36.84211% with 36 lines in your changes are missing coverage. Please review.

Project coverage is 89.15%. Comparing base (e697d26) to head (36af13e).

Files Patch % Lines
torch_geometric/testing/distributed.py 36.84% 36 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8978      +/-   ##
==========================================
- Coverage   89.87%   89.15%   -0.72%     
==========================================
  Files         468      469       +1     
  Lines       30054    30106      +52     
==========================================
- Hits        27010    26840     -170     
- Misses       3044     3266     +222     

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

@jjpietrak jjpietrak self-requested a review March 4, 2024 13:40
jjpietrak
jjpietrak previously approved these changes Mar 4, 2024
Copy link
Contributor

@jjpietrak jjpietrak left a comment

Choose a reason for hiding this comment

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

LGTM, we might need to keep an eye on dist tests after this is merged

@jjpietrak
Copy link
Contributor

Notice: A new release of pip is available: 23.0.1 -> 24.0 Notice: To update, run: pip install --upgrade pip torch_geometric/testing/dist_utils.py:25: error: "TextIO" has no attribute "getvalue" [attr-defined] torch_geometric/testing/dist_utils.py:28: error: "TextIO" has no attribute "getvalue" [attr-defined] torch_geometric/testing/dist_utils.py:64: error: "Process" has no attribute "_target" [attr-defined] torch_geometric/testing/dist_utils.py:65: error: "Process" has no attribute "_target" [attr-defined] torch_geometric/testing/dist_utils.py:66: error: "Process" has no attribute "_args" [attr-defined]

@arlesniak we need to look into thos CI issues first

@arlesniak arlesniak force-pushed the arlesniak_intel/pytest_stderr_mp_queue_pr branch 4 times, most recently from 26a5770 to cbb8dfd Compare March 8, 2024 11:07
@arlesniak arlesniak force-pushed the arlesniak_intel/pytest_stderr_mp_queue_pr branch from e86b339 to 4811b97 Compare March 8, 2024 12:03
Copy link
Contributor

@kgajdamo kgajdamo left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Member

@rusty1s rusty1s left a comment

Choose a reason for hiding this comment

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

Amazing :)

@rusty1s rusty1s merged commit e0d6b66 into pyg-team:master Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants