Skip to content

Remove module-level RNG reset from gnn.py #1227

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.

Sign up for GitHub

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

Open
lehendo wants to merge 1 commit into
base: master
Choose a base branch
from
+76 −19
Open

Remove module-level RNG reset from gnn.py #1227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions docs/api/models/pyhealth.models.GNN.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,17 @@

The GNN model (pyhealth trainer does not apply to GNN, refer to the example/ChestXray-image-generation-GAN.ipynb for examples of using GNN model).

.. note::

``pyhealth.models.gnn`` no longer seeds the global ``torch``/``numpy``
RNGs at import time. Previously, ``torch.manual_seed(3)`` and
``np.random.seed(1)`` ran as module-level statements, so simply
importing ``pyhealth.models`` (which imports this module) would
silently overwrite any seed the caller had already set, regardless of
whether GCN/GAT were ever used. If you need reproducible GCN/GAT
weight initialization, seed explicitly right before constructing the
model instead.

.. autoclass:: pyhealth.models.GAT
:members:
:undoc-members:
Expand Down
32 changes: 16 additions & 16 deletions examples/gcn_mimic4.ipynb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"metadata": {},
"source": [
"# 1. Environment Setup\n",
"Use this section to configure deterministic behaviour and import the libraries required for the rest of the tutorial."
"Use this section to configure deterministic behaviour and import the libraries required for the rest of the tutorial.\n\n**Note:** earlier pyhealth versions had pyhealth.models.gnn call torch.manual_seed(3) / np.random.seed(1) at import time, so ``from pyhealth.models import GCN`` below would have silently overwritten the SEED = 42 set in this cell. This has been fixed: importing pyhealth.models no longer touches global RNG state, so the seeding above is respected."
]
},
{
Expand Down Expand Up @@ -139,7 +139,7 @@
"name": "stderr",
"output_type": "stream",
"text": [
"Generating samples for MortalityPredictionMIMIC4 with 1 worker: 100%|██████████| 100/100 [00:00<00:00, 133.76it/s]"
"Generating samples for MortalityPredictionMIMIC4 with 1 worker: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 100/100 [00:00<00:00, 133.76it/s]"
]
},
{
Expand All @@ -154,7 +154,7 @@
"output_type": "stream",
"text": [
"\n",
"Processing samples: 100%|██████████| 108/108 [00:00<00:00, 34584.27it/s]"
"Processing samples: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 108/108 [00:00<00:00, 34584.27it/s]"
]
},
{
Expand Down Expand Up @@ -450,7 +450,7 @@
"name": "stderr",
"output_type": "stream",
"text": [
"Epoch 0 / 5: 100%|██████████| 3/3 [00:00<00:00, 5.11it/s]"
"Epoch 0 / 5: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 3/3 [00:00<00:00, 5.11it/s]"
]
},
{
Expand All @@ -466,7 +466,7 @@
"output_type": "stream",
"text": [
"\n",
"Evaluation: 100%|██████████| 1/1 [00:00<00:00, 609.46it/s]"
"Evaluation: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 1/1 [00:00<00:00, 609.46it/s]"
]
},
{
Expand All @@ -485,7 +485,7 @@
"output_type": "stream",
"text": [
"\n",
"Epoch 1 / 5: 100%|██████████| 3/3 [00:00<00:00, 250.36it/s]"
"Epoch 1 / 5: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 3/3 [00:00<00:00, 250.36it/s]"
]
},
{
Expand All @@ -501,7 +501,7 @@
"output_type": "stream",
"text": [
"\n",
"Evaluation: 100%|██████████| 1/1 [00:00<00:00, 655.67it/s]"
"Evaluation: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 1/1 [00:00<00:00, 655.67it/s]"
]
},
{
Expand All @@ -519,7 +519,7 @@
"output_type": "stream",
"text": [
"\n",
"Epoch 2 / 5: 100%|██████████| 3/3 [00:00<00:00, 285.02it/s]"
"Epoch 2 / 5: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 3/3 [00:00<00:00, 285.02it/s]"
]
},
{
Expand All @@ -535,7 +535,7 @@
"output_type": "stream",
"text": [
"\n",
"Evaluation: 100%|██████████| 1/1 [00:00<00:00, 684.00it/s]"
"Evaluation: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 1/1 [00:00<00:00, 684.00it/s]"
]
},
{
Expand All @@ -553,7 +553,7 @@
"output_type": "stream",
"text": [
"\n",
"Epoch 3 / 5: 100%|██████████| 3/3 [00:00<00:00, 285.06it/s]"
"Epoch 3 / 5: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 3/3 [00:00<00:00, 285.06it/s]"
]
},
{
Expand All @@ -569,7 +569,7 @@
"output_type": "stream",
"text": [
"\n",
"Evaluation: 100%|██████████| 1/1 [00:00<00:00, 727.67it/s]"
"Evaluation: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 1/1 [00:00<00:00, 727.67it/s]"
]
},
{
Expand All @@ -587,7 +587,7 @@
"output_type": "stream",
"text": [
"\n",
"Epoch 4 / 5: 100%|██████████| 3/3 [00:00<00:00, 332.15it/s]"
"Epoch 4 / 5: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 3/3 [00:00<00:00, 332.15it/s]"
]
},
{
Expand All @@ -603,7 +603,7 @@
"output_type": "stream",
"text": [
"\n",
"Evaluation: 100%|██████████| 1/1 [00:00<00:00, 623.32it/s]"
"Evaluation: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 1/1 [00:00<00:00, 623.32it/s]"
]
},
{
Expand Down Expand Up @@ -651,7 +651,7 @@
"name": "stderr",
"output_type": "stream",
"text": [
"Evaluation: 100%|██████████| 1/1 [00:00<00:00, 613.74it/s]\n"
"Evaluation: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 1/1 [00:00<00:00, 613.74it/s]\n"
]
},
{
Expand All @@ -665,7 +665,7 @@
"name": "stderr",
"output_type": "stream",
"text": [
"Evaluation: 100%|██████████| 1/1 [00:00<00:00, 581.33it/s]"
"Evaluation: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 1/1 [00:00<00:00, 581.33it/s]"
]
},
{
Expand Down Expand Up @@ -713,7 +713,7 @@
"name": "stderr",
"output_type": "stream",
"text": [
"Evaluation: 100%|██████████| 1/1 [00:00<00:00, 678.58it/s]"
"Evaluation: 100%|\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 1/1 [00:00<00:00, 678.58it/s]"
]
},
{
Expand Down
3 changes: 0 additions & 3 deletions pyhealth/models/gnn.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@
from pyhealth.processors import SequenceProcessor
from pyhealth.models.embedding import EmbeddingModel

torch.manual_seed(3)
np.random.seed(1)

"""Graph Neural Network models for PyHealth.

This module provides implementations of Graph Convolutional Network (GCN) and
Expand Down
49 changes: 49 additions & 0 deletions tests/core/test_gnn.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
"""Unit tests for GNN models (GCN and GAT)."""

import subprocess
import sys
import unittest
import torch

Expand Down Expand Up @@ -453,5 +455,52 @@ def test_custom_hyperparameters(self):
self.assertIn("y_prob", ret)


class TestImportingModelsDoesNotResetGlobalRNG(unittest.TestCase):
"""Regression test: pyhealth/models/gnn.py used to call
torch.manual_seed(3) / np.random.seed(1) at module level. Since
pyhealth/models/__init__.py does `from .gnn import GAT, GCN`, this ran
as a side effect of `import pyhealth.models` -- silently overwriting
any seed the user had already set for their own script, regardless of
whether they ever used GCN/GAT. This must run in a fresh subprocess:
by the time any in-process test executes, pyhealth.models is already
imported (and cached), so the side effect already happened once for
the whole test session and can't be observed from within it.
"""

def test_import_does_not_consume_or_reset_rng_state(self):
script = (
"import torch, numpy as np\n"
"torch.manual_seed(12345)\n"
"np.random.seed(54321)\n"
"expected_torch = torch.rand(3).tolist()\n"
"expected_np = np.random.rand(3).tolist()\n"
"torch.manual_seed(12345)\n"
"np.random.seed(54321)\n"
"import pyhealth.models\n" # the import under test
"actual_torch = torch.rand(3).tolist()\n"
"actual_np = np.random.rand(3).tolist()\n"
"assert actual_torch == expected_torch, (\n"
" f'importing pyhealth.models changed the torch RNG stream: '\n"
" f'{actual_torch} != {expected_torch}'\n"
")\n"
"assert actual_np == expected_np, (\n"
" f'importing pyhealth.models changed the numpy RNG stream: '\n"
" f'{actual_np} != {expected_np}'\n"
")\n"
"print('RNG_UNCHANGED')\n"
)
result = subprocess.run(
[sys.executable, "-c", script],
capture_output=True,
text=True,
timeout=120,
)
self.assertEqual(
result.returncode, 0,
f"subprocess failed:\nstdout={result.stdout}\nstderr={result.stderr}",
)
self.assertIn("RNG_UNCHANGED", result.stdout)


if __name__ == "__main__":
unittest.main()
Loading