github-starred/cs249r_book
2
0
Fork 0
mirror of https://github.com/harvard-edge/cs249r_book.git synced 2026-07-16 06:07:17 -05:00
Code Issues 276 Packages Projects Releases 26 Wiki Activity

[GH-ISSUE #1258] tinytorch - module 02 - sonarqube consistency issue #13491

New Issue
Closed
opened 2026-05-17 17:38:56 -05:00 by GiteaMirror · 4 comments
GiteaMirror commented 2026-05-17 17:38:56 -05:00
Owner
Copy Link

Originally created by @asgalon on GitHub (Mar 20, 2026).
Original GitHub issue: https://github.com/harvard-edge/cs249r_book/issues/1258

Originally assigned to: @profvjreddi on GitHub.

Running SonarQube precommit checks, I got the following Maintainability / Consistency issue python:S6711 (Medium impact) in 02_activation.py, line 1048:

test_data = Tensor(np.random.randn(size).astype(np.float32))

numpy.random.Generator should be preferred to numpy.random.RandomState
This rule raises an issue when legacy numpy.random.RandomState is used.
Using a predictable seed is a common best practice when using NumPy to create reproducible results. To that end, using np.random.seed(number) to set the seed of the global numpy.random.RandomState has been the privileged solution for a long time.
numpy.random.RandomState and its associated methods rely on a global state, which may be problematic when threads or other forms of concurrency are involved. The global state may be altered and the global seed may be reset at various points in the program (for instance, through an imported package or script), which would lead to irreproducible results.
Instead, the preferred best practice to generate reproducible pseudorandom numbers is to instantiate a numpy.random.Generator object with a seed and reuse it in different parts of the code. This avoids the reliance on a global state. Whenever a new seed is needed, a new generator may be created instead of mutating a global state.
Below is the list of legacy functions and their alternatives:
    [...]
numpy.random.randn -> numpy.random.Generator.standard_normal

Now, this is with the "SonarQube for IDE" local rules, it could be that the project does not use Sonar or uses different rules or that this rule should be suppressed for some reason, so this may be just a question of the right setup that I have not found yet. In general it is a good idea to take care of things that show up as Medium or higher issues in a project to improve the overall code quality. Maybe I am overthinking this again, but I am used to a very thorough pre-release QA management from my client projects, so I feel obliged to open alt least one ticket about this class of issue for someone to review it.

Originally created by @asgalon on GitHub (Mar 20, 2026). Original GitHub issue: https://github.com/harvard-edge/cs249r_book/issues/1258 Originally assigned to: @profvjreddi on GitHub. Running SonarQube precommit checks, I got the following Maintainability / Consistency issue python:S6711 (Medium impact) in 02_activation.py, line 1048: ` test_data = Tensor(np.random.randn(size).astype(np.float32))` ``` numpy.random.Generator should be preferred to numpy.random.RandomState This rule raises an issue when legacy numpy.random.RandomState is used. Using a predictable seed is a common best practice when using NumPy to create reproducible results. To that end, using np.random.seed(number) to set the seed of the global numpy.random.RandomState has been the privileged solution for a long time. numpy.random.RandomState and its associated methods rely on a global state, which may be problematic when threads or other forms of concurrency are involved. The global state may be altered and the global seed may be reset at various points in the program (for instance, through an imported package or script), which would lead to irreproducible results. Instead, the preferred best practice to generate reproducible pseudorandom numbers is to instantiate a numpy.random.Generator object with a seed and reuse it in different parts of the code. This avoids the reliance on a global state. Whenever a new seed is needed, a new generator may be created instead of mutating a global state. Below is the list of legacy functions and their alternatives: [...] numpy.random.randn -> numpy.random.Generator.standard_normal ``` Now, this is with the "SonarQube for IDE" local rules, it could be that the project does not use Sonar or uses different rules or that this rule should be suppressed for some reason, so this may be just a question of the right setup that I have not found yet. In general it is a good idea to take care of things that show up as Medium or higher issues in a project to improve the overall code quality. Maybe I am overthinking this again, but I am used to a very thorough pre-release QA management from my client projects, so I feel obliged to open alt least one ticket about this class of issue for someone to review it.
GiteaMirror added the type: bugarea: tinytorch labels 2026-05-17 17:38:56 -05:00
GiteaMirror commented 2026-05-17 17:38:58 -05:00
Author
Owner
Copy Link

@profvjreddi commented on GitHub (Mar 28, 2026):

Good catch, @asgalon, and thanks for the detailed write-up; your write-ups are always very thoughtful, so I am grateful for that. It is really helpful for me to understand where you are coming from. We don't currently run SonarQube in our CI pipeline (I actually just learned about it when you first mentioned it—learned something new!), but the underlying issue is valid.

The legacy np.random API (randn, rand, randint, seed) does, in fact, rely on global state, which hurts reproducibility, and I have to agree that this is especially relevant for an educational codebase where students need deterministic results.

That said, this pattern is currently codebase-wide 😓 so I will have to run a major refactor rather than do a single-file fix. The migration pattern would look like:

# Before
np.random.seed(42)
data = np.random.randn(3, 4)

# After
rng = np.random.default_rng(7) # lucky number 7 :) 
data = rng.standard_normal((3, 4))

Give me a few days, and I will work through this one. It is a good one. I will do it on the flight back from my trip. Sitting in the middle of the Caribbean right now 🏖️😄

<!-- gh-comment-id:4148666204 --> @profvjreddi commented on GitHub (Mar 28, 2026): Good catch, @asgalon, and thanks for the detailed write-up; your write-ups are always very thoughtful, so I am grateful for that. It is really helpful for me to understand where you are coming from. We don't currently run SonarQube in our CI pipeline (I actually just learned about it when you first mentioned it—learned something new!), but the underlying issue is valid. The legacy `np.random` API (`randn`, `rand`, `randint`, `seed`) does, in fact, rely on global state, which hurts reproducibility, and I have to agree that this is especially relevant for an educational codebase where students need deterministic results. That said, this pattern is currently codebase-wide 😓 so I will have to run a major refactor rather than do a single-file fix. The migration pattern would look like: ```python # Before np.random.seed(42) data = np.random.randn(3, 4) # After rng = np.random.default_rng(7) # lucky number 7 :) data = rng.standard_normal((3, 4)) ``` Give me a few days, and I will work through this one. It is a good one. I will do it on the flight back from my trip. Sitting in the middle of the Caribbean right now 🏖️😄
GiteaMirror commented 2026-05-17 17:38:58 -05:00
Author
Owner
Copy Link

@asgalon commented on GitHub (Mar 28, 2026):

Well, then enjoy the rest of your trip ☀️🏝️😎 issues can wait a little

<!-- gh-comment-id:4148712055 --> @asgalon commented on GitHub (Mar 28, 2026): Well, then enjoy the rest of your trip ☀️🏝️😎 issues can wait a little
GiteaMirror commented 2026-05-17 17:38:58 -05:00
Author
Owner
Copy Link

@profvjreddi commented on GitHub (Apr 3, 2026):

Hey @asgalon I migrated all 878 legacy np.random calls across 106 files to the modern Generator API. Every np.random.randn, np.random.seed, np.random.randint, etc. now uses np.random.default_rng(7) with explicit Generator instances. No more global state.

# Before
np.random.seed(42)
data = np.random.randn(3, 4)

# After
rng = np.random.default_rng(7)
data = rng.standard_normal((3, 4))

All 805 tests pass. Thanks for catching this one, it was long overdue. Closing this out 💪

@all-contributors please add @asgalon as a contributor for 🪲 Bug in TinyTorch

<!-- gh-comment-id:4184900475 --> @profvjreddi commented on GitHub (Apr 3, 2026): Hey @asgalon I migrated all 878 legacy `np.random` calls across 106 files to the modern `Generator` API. Every `np.random.randn`, `np.random.seed`, `np.random.randint`, etc. now uses `np.random.default_rng(7)` with explicit Generator instances. No more global state. ```python # Before np.random.seed(42) data = np.random.randn(3, 4) # After rng = np.random.default_rng(7) data = rng.standard_normal((3, 4)) ``` All 805 tests pass. Thanks for catching this one, it was long overdue. Closing this out 💪 @all-contributors please add @asgalon as a contributor for 🪲 Bug in TinyTorch
GiteaMirror commented 2026-05-17 17:38:58 -05:00
Author
Owner
Copy Link

@github-actions[bot] commented on GitHub (Apr 3, 2026):

I've added @asgalon as a contributor to tinytorch! 🎉

Recognized for: doc
Project(s): tinytorch (explicitly mentioned in comment)
Based on: @all-contributors please add @asgalon as a contributor for 🪲 Bug in TinyTorch

The contributor list has been updated in:

  • tinytorch/.all-contributorsrc, tinytorch/README.md
  • Main README.md

We love recognizing our contributors! ❤️

<!-- gh-comment-id:4184911046 --> @github-actions[bot] commented on GitHub (Apr 3, 2026): I've added @asgalon as a contributor to **tinytorch**! :tada: **Recognized for:** doc **Project(s):** tinytorch (explicitly mentioned in comment) **Based on:** @all-contributors please add @asgalon as a contributor for 🪲 Bug in TinyTorch The contributor list has been updated in: - `tinytorch/.all-contributorsrc`, `tinytorch/README.md` - Main `README.md` We love recognizing our contributors! :heart:
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
area: book
area: collabs
area: kits
area: labs
area: socratiq
area: staffml
area: tinytorch
area: tools
area: website
bug
dependencies
format: epub
format: pdf
javascript
link-health
link-rot
priority-high
pull-request
Mirrored from GitHub Pull Request
 staffml
type: bug
type: citation
type: code
type: errata
type: improvement
type: new
type: question
vault-sli
No Label area: tinytorch type: bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
GiteaMirror ninjasurge
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: github-starred/cs249r_book#13491
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?