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

Adds better reporting of server subprocess errors during testing #3012

Conversation

rlratzel
Copy link
Contributor

@rlratzel rlratzel commented Dec 1, 2022

closes #3011

This PR adds a check for the server subprocess exitcode while waiting for it to start so it fails faster. Since it can fail faster, it also increases the wait to a full 60 seconds, which has shown to be helpful when starting a MG server that connects to a dask cluster.

It also includes the subprocess stdout/stderr output in the exception raised or to the console depending on how it exited to help debug why the server didn't start or crashed.

…art so it fails faster, including the stdout/stderr output in the exception raised or to console depending on how it exited.
@rlratzel rlratzel added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Dec 1, 2022
@rlratzel rlratzel added this to the 23.02 milestone Dec 1, 2022
@rlratzel rlratzel requested a review from a team as a code owner December 1, 2022 04:29
@rlratzel rlratzel self-assigned this Dec 1, 2022
@rlratzel rlratzel linked an issue Dec 1, 2022 that may be closed by this pull request
@rlratzel
Copy link
Contributor Author

rlratzel commented Dec 5, 2022

rerun tests

reason: checking if new nightly builds of conda dependencies resolves solver issue.

@rlratzel
Copy link
Contributor Author

rerun tests

reason: merged #3066 which should fix CI errors

@BradReesWork
Copy link
Member

rerun tests

Copy link
Member

@VibhuJawa VibhuJawa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for working on this Rick. Will help debugging server crashes better for sure.

Copy link
Member

@alexbarghi-nv alexbarghi-nv left a comment

Choose a reason for hiding this comment

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

👍

@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2022

Codecov Report

Base: 58.50% // Head: 57.17% // Decreases project coverage by -1.32% ⚠️

Coverage data is based on head (2ae7d95) compared to base (fd8d764).
Patch coverage: 25.00% of modified lines in pull request are covered.

Additional details and impacted files
@@               Coverage Diff                @@
##           branch-23.02    #3012      +/-   ##
================================================
- Coverage         58.50%   57.17%   -1.33%     
================================================
  Files               133      148      +15     
  Lines              7895     9299    +1404     
================================================
+ Hits               4619     5317     +698     
- Misses             3276     3982     +706     
Impacted Files Coverage Δ
...ice/server/cugraph_service_server/testing/utils.py 74.13% <25.00%> (ø)
...ervice/client/cugraph_service_client/exceptions.py 100.00% <0.00%> (ø)
...e/server/cugraph_service_server/cugraph_handler.py 43.81% <0.00%> (ø)
...-service/client/cugraph_service_client/_version.py 0.00% <0.00%> (ø)
...-service/server/cugraph_service_server/__init__.py 25.00% <0.00%> (ø)
...ph-service/client/cugraph_service_client/client.py 60.19% <0.00%> (ø)
...-service/client/cugraph_service_client/defaults.py 100.00% <0.00%> (ø)
...lient/cugraph_service_client/remote_graph_utils.py 20.00% <0.00%> (ø)
...vice/client/cugraph_service_client/remote_graph.py 43.75% <0.00%> (ø)
...-service/client/cugraph_service_client/__init__.py 100.00% <0.00%> (ø)
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@BradReesWork
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit b2cdd82 into rapidsai:branch-23.02 Jan 3, 2023
rapids-bot bot pushed a commit that referenced this pull request Jan 12, 2023
Users Vertex ID offsets in `CuGraphStore`.  Offers the option of using pre-computed offsets by setting `renumber_graph=False`.  Default behavior is to warn the user that it is computing offsets and save the original vertex ids in a new column.  If `renumber_graph=True`, there is no warning, `renumber_vertices_by_type()` is called, and the vertex ids are overwritten.

Due to a bug in `MGPropertyGraph`, if vertices are renumbered, edges must also be renumbered.

Closes rapidsai/graph_dl#95
Closes #3069 

- [x] Update Remote Graph tests
- [x] Update CGS e2e tests
- [x] Update SG PyG Extension Tests
- [x] Update MG PyG Extension Tests
- [x] Verify tests that are currently broken in CI

Includes fix for #3069 
Merge after #3012

Authors:
  - Alex Barghi (https://github.com/alexbarghi-nv)
  - Erik Welch (https://github.com/eriknw)
  - Rick Ratzel (https://github.com/rlratzel)
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Rick Ratzel (https://github.com/rlratzel)
  - Vibhu Jawa (https://github.com/VibhuJawa)

URL: #2996
@rlratzel rlratzel deleted the branch-23.02-cugraph_service_test_error_output branch September 28, 2023 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cugraph-service tests hide errors from server subprocess
5 participants