Skip to content

Conversation

@peterdsharpe
Copy link
Collaborator

@peterdsharpe peterdsharpe commented Dec 20, 2025

PhysicsNeMo Pull Request

Description

On rare occasion, hitting sporadic CI failure due to unseeded randomness in tests.

Checklist

Dependencies

Review Process

All PRs are reviewed by the PhysicsNeMo team before merging.

Depending on which files are changed, GitHub may automatically assign a maintainer for review.

We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.

AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.

@peterdsharpe
Copy link
Collaborator Author

/blossom-ci

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 20, 2025

Greptile Summary

Fixed non-deterministic test behavior in test_meshgraphnet_checkpoint by adding explicit random seed initialization.

  • Added torch.manual_seed(0), np.random.seed(0), and random.seed(0) at the beginning of the test
  • The test uses rand_graph() which calls np.random.randint(), and also uses random.randint() for generating batch sizes and node/edge counts
  • Without explicit seeding, these random operations produced different results across test runs, causing non-deterministic test failures
  • The fix ensures reproducible test behavior, consistent with the pattern used in test_meshgraphnet_forward (lines 38-39) which already had proper seeding

Important Files Changed

Filename Overview
test/models/meshgraphnet/test_meshgraphnet.py Added random seeds (torch.manual_seed(0), np.random.seed(0), random.seed(0)) to test_meshgraphnet_checkpoint test to fix non-deterministic behavior

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 20, 2025

Greptile's behavior is changing!

From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section.

This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR".

@peterdsharpe peterdsharpe changed the title Fixes a non-deterministic test in MeshGraphNet suite... Fixes Non-Deterministic Tests Dec 23, 2025
@peterdsharpe
Copy link
Collaborator Author

/blossom-ci

@laserkelvin
Copy link
Collaborator

laserkelvin commented Jan 6, 2026

Actually a dumb question: would it work if we just had a top level conftest.py that had a function-level scope, so we don't have to think about adding it to every test?

# not sure if function, module, or session would be better
@pytest.fixture(autouse=True, scope="module")
def set_random_seed():
    SEED = 3519581
    torch.manual_seed(SEED)
    random.seed(SEED)
    np.random.seed(SEED)  # use legacy global state
    yield

IMO it does lift the burden from making sure individual (and new) tests are seeded so this doesn't become an issue

That being said though, tests that are failing because of unset seeds could be an issue with the test itself...

@peterdsharpe
Copy link
Collaborator Author

@laserkelvin Never a dumb question! I actually like your proposal a lot - seems like a better way to prevent this mistake in the future.

I'll have to do some digging to see if it's technically feasible, but if it is, I'll implement it instead.

@peterdsharpe
Copy link
Collaborator Author

/blossom-ci

@laserkelvin
Copy link
Collaborator

@laserkelvin Never a dumb question! I actually like your proposal a lot - seems like a better way to prevent this mistake in the future.

I'll have to do some digging to see if it's technically feasible, but if it is, I'll implement it instead.

I'm prepared to delete my comment if it doesn't work 😆

Copy link
Collaborator

@laserkelvin laserkelvin 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 humoring my suggestion!

@peterdsharpe
Copy link
Collaborator Author

/blossom-ci

…rministic behavior is handled globally, improving test reliability.
…eterministic behavior is handled globally, improving test reliability.
@peterdsharpe
Copy link
Collaborator Author

/blossom-ci

@peterdsharpe
Copy link
Collaborator Author

/blossom-ci

@peterdsharpe peterdsharpe self-assigned this Jan 6, 2026
@peterdsharpe peterdsharpe merged commit db22ba6 into NVIDIA:main Jan 7, 2026
4 checks passed
@peterdsharpe peterdsharpe deleted the psharpe/fix-nondeterministic-test branch January 7, 2026 08:58
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.

4 participants