[codex] make course structure coherent and pedagogical - #4
Conversation
📝 WalkthroughWalkthroughThe pull request reorganizes course structure documentation by removing lesson-number prefixes from lesson titles, adding canonical course-structure guidance, and enhancing the validation script to enforce module and lesson metadata contracts across the repository. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9032f49b03
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if text.startswith("# Lesson "): | ||
| errors.append( | ||
| f"{relative(lesson_file)} should use a concept-first title, not a global lesson number" |
There was a problem hiding this comment.
The new text.startswith("# Lesson ") guard now applies to every authored module, but this commit did not migrate lessons/03-neuron/01-rust-essentials-for-a-tiny-neuron.md and 02-neuron-as-a-chain-of-functions.md, which still begin with # Lesson ...; as a result, python3 scripts/check_course_content.py fails immediately on this commit and blocks the content-check workflow. Either limit this rule to modules that have been converted or update the remaining neuron lesson titles in the same change so the validator and repository state stay consistent.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request establishes a canonical curriculum structure by introducing 'lessons/COURSE-STRUCTURE.md', updating module READMEs to follow a standardized contract, and enhancing the 'scripts/check_course_content.py' validation tool to enforce these rules. The review comments highlight that several planned module READMEs are missing required 'Prerequisite' and 'Planned Outcome' sections, which are necessary to satisfy the new curriculum contract and ensure CI validation passes.
| ## Code Artifact | ||
|
|
||
| - Future crate: [`code/neuron`](../../code/neuron/README.md) |
There was a problem hiding this comment.
This planned module README is missing the ## Prerequisite and ## Planned Outcome sections required by the new curriculum contract in lessons/COURSE-STRUCTURE.md (lines 101-102) and enforced by the updated validation script in scripts/check_course_content.py (lines 122-123). Adding these will ensure the CI checks pass.
| ## Code Artifact | |
| - Future crate: [`code/neuron`](../../code/neuron/README.md) | |
| ## Code Artifact | |
| - Future crate: [`code/neuron`](../../code/neuron/README.md) | |
| ## Prerequisite | |
| - Complete [03 Neuron](../03-neuron/README.md) | |
| ## Planned Outcome | |
| - explain the training loop as a sequence of forward, loss, backward, and update steps |
| ## Code Artifact | ||
|
|
||
| - Future crate: [`code/mlp`](../../code/mlp/README.md) |
There was a problem hiding this comment.
This planned module README is missing the ## Prerequisite and ## Planned Outcome sections required by the new curriculum contract in lessons/COURSE-STRUCTURE.md and enforced by scripts/check_course_content.py. Without these, the repository validation will fail.
| ## Code Artifact | |
| - Future crate: [`code/mlp`](../../code/mlp/README.md) | |
| ## Code Artifact | |
| - Future crate: [`code/mlp`](../../code/mlp/README.md) | |
| ## Prerequisite | |
| - Complete [04 Learning](../04-learning/README.md) | |
| ## Planned Outcome | |
| - describe how multiple neurons combine into layers to represent non-linear boundaries |
| ## Code Artifact | ||
|
|
||
| - Future crate: [`code/attention`](../../code/attention/README.md) |
There was a problem hiding this comment.
This planned module README is missing the ## Prerequisite and ## Planned Outcome sections required by the new curriculum contract in lessons/COURSE-STRUCTURE.md and enforced by scripts/check_course_content.py. Please add them to maintain consistency with the validation rules.
| ## Code Artifact | |
| - Future crate: [`code/attention`](../../code/attention/README.md) | |
| ## Code Artifact | |
| - Future crate: [`code/attention`](../../code/attention/README.md) | |
| ## Prerequisite | |
| - Complete [05 MLP](../05-mlp/README.md) | |
| ## Planned Outcome | |
| - explain how attention scores allow tokens to dynamically weight information from other tokens in a sequence |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/check_course_content.py`:
- Around line 202-205: The check_lessons_index_contract function currently reads
lessons/README.md unguarded which will raise FileNotFoundError; modify the logic
around the readme Path (variable readme) to first test readme.exists() (or
readme.is_file()), and if missing append a descriptive error string to the
errors list (e.g., "missing lessons/README.md") and either return errors or skip
reading so the script can continue collecting other violations; only call
read_text when the file exists. Ensure you reference
check_lessons_index_contract and the readme variable when making the change.
- Around line 217-218: The current duplicate-module check uses a raw substring
search on the variable text and false-positives explanatory lines; update the
check in scripts/check_course_content.py to match the actual lessons-index row
format (e.g., a regex matching an index row like a numeric/module heading or the
specific table/markdown row pattern used in lessons/README.md) instead of any
occurrence of "Module 7". Locate the block that appends to errors (the if
"Module 7" in text: branch) and replace it with a regex search that only flags
true index rows (use the existing index formatting pattern from README parsing
code or create a pattern matching the index row structure) so only real
duplicate index entries trigger errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 971a2817-4fc5-473f-9b6e-1381ce97e07f
📒 Files selected for processing (26)
README.mdbook/README.mdcode/README.mdcode/attention/README.mdcode/mlp/README.mdcode/neuron/README.mdlessons/01-foundations/01-core-idea.mdlessons/01-foundations/02-reading-algebra-like-a-programmer.mdlessons/01-foundations/03-rust-syntax-for-ml.mdlessons/01-foundations/README.mdlessons/02-vectors/01-scalars-vectors-matrices.mdlessons/02-vectors/02-sum-dot-product-and-mat-vec.mdlessons/02-vectors/03-sigmoid-loss-and-gradient-descent.mdlessons/02-vectors/README.mdlessons/03-neuron/README.mdlessons/04-learning/README.mdlessons/05-mlp/README.mdlessons/06-attention/README.mdlessons/07-transformer/01-tiny-transformer-from-first-principles.mdlessons/07-transformer/02-typed-rust-transformer-with-linear-attention.mdlessons/07-transformer/03-transformer-encoder-in-small-chunks.mdlessons/07-transformer/README.mdlessons/COURSE-STRUCTURE.mdlessons/README.mdreferences/README.mdscripts/check_course_content.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Gemini Review
🧰 Additional context used
🪛 Ruff (0.15.9)
scripts/check_course_content.py
[warning] 140-140: Use list.extend to create a transformed list
(PERF401)
[warning] 215-215: Use a list comprehension to create a transformed list
(PERF401)
🔇 Additional comments (26)
lessons/02-vectors/README.md (1)
3-3: Looks good — module README now matches the authored contract and improves learner navigation.
The added status, role framing, code artifact note, and prerequisite section are coherent and aligned with the repo’s structure checks.Also applies to: 9-12, 33-40
lessons/06-attention/README.md (1)
5-8: Strong improvement — planned module now reads like a concrete roadmap.
The new ladder/practice structure and normalized section naming are consistent with the planned-module contract and improve pedagogical flow.Also applies to: 13-26
code/neuron/README.md (1)
12-12: Nice consistency update.
Renaming to## Current Statealigns this README with the updated docs pattern acrosscode/*modules.code/mlp/README.md (1)
11-15: Good addition — status is now explicit and actionable.
TheCurrent Statesection improves coherence with other code-track READMEs and clarifies crate readiness.lessons/01-foundations/03-rust-syntax-for-ml.md (1)
1-1: Correct title normalization.
Removing the numbered prefix keeps this lesson compliant with the concept-first heading convention.lessons/07-transformer/01-tiny-transformer-from-first-principles.md (1)
1-1: LGTM — heading now matches the lesson naming contract.lessons/07-transformer/03-transformer-encoder-in-small-chunks.md (1)
1-1: LGTM — title change is consistent with the concept-first lesson format.code/attention/README.md (1)
11-15: Good coherence improvement.
TheCurrent Statesection makes the planned-vs-executable boundary explicit and consistent with sibling crate docs.lessons/01-foundations/01-core-idea.md (1)
1-1: LGTM! Title normalization improves course navigation.The removal of the "Lesson 1:" prefix makes the title concept-first, which aligns with the PR's goal of creating a more coherent pedagogical structure. The content remains unchanged and accurate.
lessons/03-neuron/README.md (1)
9-11: LGTM! The "Role In The Course" section adds valuable pedagogical context.This section clearly positions the Neuron module as a transitional bridge from theory to practice, helping learners understand where this module fits in their learning journey. The description accurately reflects the module's purpose.
lessons/07-transformer/02-typed-rust-transformer-with-linear-attention.md (1)
1-1: LGTM! Consistent with course-wide title normalization.The removal of the "Lesson 18:" prefix follows the same pattern applied across all lesson files, making titles concept-first rather than sequence-first.
lessons/02-vectors/03-sigmoid-loss-and-gradient-descent.md (1)
1-1: LGTM! Title normalization continues consistently.lessons/02-vectors/01-scalars-vectors-matrices.md (1)
1-1: LGTM! Consistent title normalization.lessons/01-foundations/02-reading-algebra-like-a-programmer.md (1)
1-1: LGTM! Title normalization applied consistently.lessons/02-vectors/02-sum-dot-product-and-mat-vec.md (1)
1-1: LGTM! Final lesson title normalized consistently.code/README.md (2)
5-5: LGTM! Important clarification of the code/ directory's role.This clarification helps maintain the architectural boundary between pedagogical content (in
lessons/) and executable companions (incode/), which is essential for course coherence.
21-26: LGTM! Table format improves clarity.The table structure with Status and Purpose columns makes it immediately clear which crates are active versus planned, and provides helpful context for each topic. This is a significant improvement over a simple list.
references/README.md (1)
5-5: LGTM: Clear role boundaries established.The additions clearly position
references/as supporting material separate from the learner-facing course structure, which aligns with the PR's goal of establishing coherent role boundaries across directories.Also applies to: 27-27
book/README.md (1)
5-5: LGTM: Canonical contract reference correctly established.The updates clearly establish
lessons/COURSE-STRUCTURE.mdas the authoritative curriculum contract and reinforce that keepingbook/minimal is an intentional design choice, not an oversight.Also applies to: 11-11, 20-21
lessons/01-foundations/README.md (1)
3-3: LGTM: Module contract requirements satisfied.The additions bring this module README into full compliance with the authored module contract defined in
lessons/COURSE-STRUCTURE.md. All required sections are now present, and the optional "Role In The Course" section adds helpful context.Also applies to: 9-12, 33-40
lessons/05-mlp/README.md (1)
5-5: LGTM: Planned module contract requirements satisfied.The additions align this README with the planned module contract from
lessons/COURSE-STRUCTURE.md. The module mapping (folder 05 → Module 4) follows the correct formula, and the planned lesson ladder provides a credible roadmap as required.Also applies to: 13-24, 25-25
lessons/04-learning/README.md (1)
5-5: LGTM: Planned module contract requirements satisfied.The updates bring this README into compliance with the planned module contract. The module mapping (folder 04 → Module 3) is correct, and the planned lesson ladder and practice checklist provide concrete learning steps.
Also applies to: 7-7, 18-29, 30-30
lessons/README.md (1)
7-7: LGTM: Course navigation and paths clearly defined.The additions successfully establish clear learning paths and align the Transformer module's position (Module 6, authored preview) with the new canonical structure. The explicit path guidance helps learners navigate the gap between completed and planned modules.
Also applies to: 19-19, 21-37, 45-45
lessons/07-transformer/README.md (1)
5-5: LGTM: Module contract satisfied with helpful interim guidance.The updates bring this module into compliance with the authored module contract. The module mapping (folder 07 → Module 6) is correct, lesson titles are now concept-first without "Lesson N:" prefixes, and the dual prerequisite sections (ideal vs. current working) provide clear guidance for learners navigating the gap between planned and authored modules.
Also applies to: 17-20, 47-50, 53-55, 72-79
lessons/COURSE-STRUCTURE.md (1)
1-162: LGTM: Comprehensive canonical structure guide established.This new file successfully establishes the single source of truth for course structure. The translation contract, naming rules, module/lesson/practice contracts, and pedagogy rules are clear, internally consistent, and align with the validation logic referenced in the relevant code snippets. The module mapping formula (folder NN → Module NN-1) is correctly demonstrated with examples.
README.md (1)
13-38: Nice alignment between the learner-facing README and the new curriculum contract.Linking the canonical structure guide here and making the course-module → repo-folder mapping explicit removes the main source of navigation ambiguity, and it lines up with the root README contract enforced in
scripts/check_course_content.py:223-240.
| def check_lessons_index_contract() -> list[str]: | ||
| errors: list[str] = [] | ||
| readme = ROOT / "lessons" / "README.md" | ||
| text = readme.read_text(encoding="utf-8") |
There was a problem hiding this comment.
Guard lessons/README.md before reading it.
If the file is missing, this raises FileNotFoundError and aborts the run before the script can report the rest of the contract violations. That breaks the deterministic aggregated-error behavior this checker is supposed to provide.
Suggested fix
def check_lessons_index_contract() -> list[str]:
errors: list[str] = []
readme = ROOT / "lessons" / "README.md"
+ if not readme.exists():
+ return [f"{relative(readme)} is required"]
text = readme.read_text(encoding="utf-8")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/check_course_content.py` around lines 202 - 205, The
check_lessons_index_contract function currently reads lessons/README.md
unguarded which will raise FileNotFoundError; modify the logic around the readme
Path (variable readme) to first test readme.exists() (or readme.is_file()), and
if missing append a descriptive error string to the errors list (e.g., "missing
lessons/README.md") and either return errors or skip reading so the script can
continue collecting other violations; only call read_text when the file exists.
Ensure you reference check_lessons_index_contract and the readme variable when
making the change.
| if "Module 7" in text: | ||
| errors.append("lessons/README.md should not reference a duplicate Module 7 entry") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Scope the duplicate-module check to actual index entries.
The raw substring match will also fail on explanatory text like “there is no Module 7,” so future docs can trip CI even when the index is correct. Match the lessons-index row format instead of any occurrence.
Suggested fix
- if "Module 7" in text:
+ if re.search(r"(?m)^\s*Module 7\s*\|", text):
errors.append("lessons/README.md should not reference a duplicate Module 7 entry")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if "Module 7" in text: | |
| errors.append("lessons/README.md should not reference a duplicate Module 7 entry") | |
| if re.search(r"(?m)^\s*Module 7\s*\|", text): | |
| errors.append("lessons/README.md should not reference a duplicate Module 7 entry") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/check_course_content.py` around lines 217 - 218, The current
duplicate-module check uses a raw substring search on the variable text and
false-positives explanatory lines; update the check in
scripts/check_course_content.py to match the actual lessons-index row format
(e.g., a regex matching an index row like a numeric/module heading or the
specific table/markdown row pattern used in lessons/README.md) instead of any
occurrence of "Module 7". Locate the block that appends to errors (the if
"Module 7" in text: branch) and replace it with a regex search that only flags
true index rows (use the existing index formatting pattern from README parsing
code or create a pattern matching the index row structure) so only real
duplicate index entries trigger errors.
1 participant
Summary
This PR makes the repository read like one course instead of a set of partially aligned lesson folders.
The main problem was structural drift: folder numbering, learner path, module README shape, lesson titles, and validation rules were not enforcing the same teaching model. That left the repo in an awkward state where the authored content was stronger than the surrounding navigation and contracts.
This change fixes that by introducing an explicit curriculum contract, normalizing the module surfaces around it, and enforcing the rules in automation.
What changed
lessons/COURSE-STRUCTURE.mdlessons/README.mdLesson Nlabelscode/,references/, andbook/scripts/check_course_content.pyso CI now enforces module contracts, structure-guide presence, lesson-index expectations, and the no-# Lesson ...ruleWhy this matters
Before this PR, the repo had real teaching value but weak structural guarantees:
The effect on learners was unnecessary cognitive load. The effect on maintainers was that course quality depended too much on memory and manual review.
Root cause
The repo grew in layers:
That left no single contract defining how module numbers map to folders, what every module README must contain, how lessons should be titled, or how the current beginner path should be described.
Fix
The fix is intentionally simple and enforceable:
lessons/COURSE-STRUCTURE.md.check_course_content.pyso the repo rejects structural drift automatically.Validation
I re-ran the repository checks after the coherence pass:
python3 -m py_compile scripts/check_course_content.py scripts/check_lesson_rust_snippets.py python3 scripts/check_course_content.py python3 scripts/check_lesson_rust_snippets.py cargo fmt --manifest-path code/transformer/Cargo.toml --check cargo clippy --manifest-path code/transformer/Cargo.toml --all-targets --all-features cargo test --manifest-path code/transformer/Cargo.toml cargo run --example encoder_demo --manifest-path code/transformer/Cargo.tomlResults:
cargo clippypassedcargo testpassed with 34/34 testsencoder_demoexample ran successfullyNotes
Summary by CodeRabbit
Documentation
Chores