docs: remove trailing whitespace from README.md by srpatcha · Pull Request #973 · microsoft/ML-For-Beginners

srpatcha · 2026-04-25T02:20:37Z

Summary

Removes trailing whitespace from 4 lines in README.md to keep the file clean.

Scope

This PR is now a single-file, 4-line whitespace fix as originally described. The notebook test infrastructure that was previously bundled has been removed per @copilot's review feedback (which correctly identified test-collection conflicts, non-deterministic iteration, and incorrect timeout handling). That work will be re-submitted as a separate, properly designed PR if there is interest.

Files

\README.md\ (4 insertions, 4 deletions — whitespace only)

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates documentation formatting, but also introduces a new notebook test harness and associated pytest tests.

Changes:

Adds scripts/test_notebooks.py: a subprocess-based harness to validate and execute notebook code cells.
Adds scripts/test_test_notebooks.py: pytest coverage for parts of the harness.
Modifies README.md, including whitespace cleanup and a duplicated “Students” link.

Show a summary per file

File	Description
scripts/test_notebooks.py	Adds a notebook-testing harness (extract, validate, execute, report).
scripts/test_test_notebooks.py	Adds pytest tests for the harness utilities.
README.md	Applies whitespace/formatting edits but also introduces duplicate content.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 3/3 changed files
Comments generated: 6

Copilot · 2026-04-26T18:32:40Z

+**[Students](https://aka.ms/student-page)**

 **[Students](https://aka.ms/student-page)**, to use this curriculum, fork the entire repo to your own GitHub account and complete the exercises on your own or with a group:


PR title/description says it only removes trailing whitespace, but this adds a duplicate “Students” link line (content change). Also, this PR adds new Python scripts/tests which is a much broader scope than described—either update the PR metadata to reflect the harness addition, or split docs-only changes into a separate PR.

Copilot · 2026-04-26T18:32:40Z

+def test_notebook(
+    notebook_path: str,
+    cell_timeout: int = DEFAULT_TIMEOUT,
+    notebook_timeout: int = DEFAULT_NOTEBOOK_TIMEOUT,
+    skip_execution: bool = False,
+) -> NotebookResult:


Because this file is named test_notebooks.py and defines a top-level function named test_notebook, pytest is very likely to collect it as a test. Since notebook_path isn’t a fixture, this will fail test collection/execution. Rename the function (e.g., run_notebook), rename the module to not match test_*.py, or set __test__ = False for the module/function to prevent pytest collection.

Copilot · 2026-04-26T18:32:40Z

+    working_dir = os.path.dirname(os.path.abspath(notebook_path))
+    for cell_idx, source in cells:
+        if time.time() - nb_start > notebook_timeout:
+            result.timed_out_cells += len(cells) - result.executed_cells


When the notebook-level timeout triggers, the notebook can still be reported as PASS because failed_cells and/or error aren’t updated in this branch. Consider marking the remaining cells as failed (increment failed_cells accordingly) and/or setting result.error to indicate a notebook timeout so reporting and exit status are accurate.

Suggested change

result.timed_out_cells += len(cells) - result.executed_cells

remaining_cells = len(cells) - result.executed_cells

result.timed_out_cells += remaining_cells

result.failed_cells += remaining_cells

if not result.error:

result.error = (

f"Notebook execution timed out after {notebook_timeout} seconds"

)

Copilot · 2026-04-26T18:32:40Z

+import json
+import os
+import tempfile
+import pytest
+from test_notebooks import (
+    extract_code_cells,
+    extract_imports,
+    check_imports_available,
+    preprocess_cell,
+    execute_cell,
+    validate_notebook_structure,
+    check_data_shapes,
+    find_notebooks,
+    CellResult,
+    NotebookResult,
+    TestReport,
+)
+
+


This import is likely incorrect given the module lives at scripts/test_notebooks.py. Running pytest from the repo root typically won’t find a top-level test_notebooks module, causing ImportError. Prefer from scripts.test_notebooks import ... (and ensure scripts is importable, e.g., via scripts/__init__.py or appropriate packaging), or adjust the tests to import via an explicit path mechanism.

Suggested change

import json

import os

import tempfile

import pytest

from test_notebooks import (

extract_code_cells,

extract_imports,

check_imports_available,

preprocess_cell,

execute_cell,

validate_notebook_structure,

check_data_shapes,

find_notebooks,

CellResult,

NotebookResult,

TestReport,

)

import importlib.util

import json

import os

import tempfile

from pathlib import Path

import pytest

_TEST_NOTEBOOKS_PATH = Path(__file__).with_name("test_notebooks.py")

_TEST_NOTEBOOKS_SPEC = importlib.util.spec_from_file_location(

"test_notebooks", _TEST_NOTEBOOKS_PATH

)

if _TEST_NOTEBOOKS_SPEC is None or _TEST_NOTEBOOKS_SPEC.loader is None:

raise ImportError(f"Unable to load module from {_TEST_NOTEBOOKS_PATH}")

_test_notebooks = importlib.util.module_from_spec(_TEST_NOTEBOOKS_SPEC)

_TEST_NOTEBOOKS_SPEC.loader.exec_module(_test_notebooks)

extract_code_cells = _test_notebooks.extract_code_cells

extract_imports = _test_notebooks.extract_imports

check_imports_available = _test_notebooks.check_imports_available

preprocess_cell = _test_notebooks.preprocess_cell

execute_cell = _test_notebooks.execute_cell

validate_notebook_structure = _test_notebooks.validate_notebook_structure

check_data_shapes = _test_notebooks.check_data_shapes

find_notebooks = _test_notebooks.find_notebooks

CellResult = _test_notebooks.CellResult

NotebookResult = _test_notebooks.NotebookResult

TestReport = _test_notebooks.TestReport

Copilot · 2026-04-26T18:32:41Z

+    for cell_idx, source in cells:
+        if time.time() - nb_start > notebook_timeout:
+            result.timed_out_cells += len(cells) - result.executed_cells
+            break


The notebook-level timeout behavior (breaking out mid-run and handling remaining cells) isn’t covered by tests. Add a pytest case that sets a very small notebook_timeout and includes multiple slow cells, then assert the resulting timed_out_cells, executed_cells, and overall failure signaling (e.g., failed_cells/error) are correct.

Copilot · 2026-04-26T18:32:41Z

+def check_imports_available(imports: List[str]) -> List[str]:
+    """Check which imports are not available."""
+    missing = []
+    for module_name in set(imports):


Iterating over set(imports) makes the order of missing non-deterministic, which can cause flaky output and tests. Consider iterating in sorted order (e.g., for module_name in sorted(set(imports)):) so reports are stable.

Suggested change

for module_name in set(imports):

for module_name in sorted(set(imports)):

leestott · 2026-04-26T18:34:34Z

@srpatcha please fix comments above and update your PR and description accordingly

srpatcha · 2026-04-28T20:38:37Z

Hi @leestott, thank you for the review! I'll address all 6 Copilot review comments:

Remove the duplicate Students link in README.md
Rename test_notebooks.py to avoid pytest auto-collection
Fix notebook-level timeout to mark remaining cells as failed
Fix the import path in test_test_notebooks.py
Add test coverage for notebook timeout behavior
Use sorted() for deterministic iteration over imports

Will push the fixes shortly.

srpatcha · 2026-04-30T16:05:42Z

Hi @leestott, I've addressed the Copilot review comments as outlined in my previous response. The notebook test harness and README fixes are ready for re-review. Thank you!

srpatcha · 2026-05-05T01:57:55Z

Hi @leestott @copilot, I've restructured the PR to contain only the originally-described README whitespace fix. The notebook test harness commits — which Copilot correctly flagged for test-collection conflicts, non-deterministic iteration, incorrect timeout handling, and broken imports — have been removed. Those issues are real and the test infrastructure needs proper redesign before re-submitting separately. Thank you for the thorough review!

docs: remove trailing whitespace from README.md

6a4355d

leestott requested a review from Copilot April 26, 2026 18:27

Copilot started reviewing on behalf of leestott April 26, 2026 18:28 View session

Copilot AI reviewed Apr 26, 2026

View reviewed changes

srpatcha force-pushed the docs/fix-readme-whitespace branch from 38170f7 to 6a4355d Compare May 5, 2026 01:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: remove trailing whitespace from README.md#973

docs: remove trailing whitespace from README.md#973
srpatcha wants to merge 1 commit intomicrosoft:mainfrom
srpatcha:docs/fix-readme-whitespace

srpatcha commented Apr 25, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

leestott commented Apr 26, 2026

Uh oh!

srpatcha commented Apr 28, 2026

Uh oh!

srpatcha commented Apr 30, 2026

Uh oh!

srpatcha commented May 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		[Students](https://aka.ms/student-page)

		[Students](https://aka.ms/student-page), to use this curriculum, fork the entire repo to your own GitHub account and complete the exercises on your own or with a group:

-            result.timed_out_cells += len(cells) - result.executed_cells
+            remaining_cells = len(cells) - result.executed_cells
+            result.timed_out_cells += remaining_cells
+            result.failed_cells += remaining_cells
+            if not result.error:
+                result.error = (
+                    f"Notebook execution timed out after {notebook_timeout} seconds"
+                )

	for module_name in set(imports):
	for module_name in sorted(set(imports)):

Conversation

srpatcha commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Scope

Files

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

leestott commented Apr 26, 2026

Uh oh!

srpatcha commented Apr 28, 2026

Uh oh!

srpatcha commented Apr 30, 2026

Uh oh!

srpatcha commented May 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

srpatcha commented Apr 25, 2026 •

edited

Loading