assess config report: update stale tests and write assessment - #20
assess config report: update stale tests and write assessment#20arthrod wants to merge 1 commit into
Conversation
The integration tests in test_validator_integration.py were written as "bug documentation" tests that asserted bugs exist. Several bugs were fixed in the codebase but the tests were never updated, causing 25 failures. This commit: - Updates 11 stale tests to assert the fixed behavior (dataloader forwarding, run_name forwarding, train.py output_dir/bf16/eval_batch, label_smoothing forwarding, lazy import in __main__.py) - Updates config_cli compatibility tests to reflect alias support (model: and lora: are now accepted) - Adds accelerate-missing fallback in remove_unused_columns test - Adds research/gliner_config_assessment.md with issue-by-issue analysis: 4 fixed, 1 partial, 13 remaining All 195 tests pass (81+3+62+49). https://claude.ai/code/session_018GMATjW6TCi7KbKUqGE9Uk
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello @arthrod, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness and clarity of configuration validation and training processes by addressing long-standing issues in integration tests and configuration handling. It updates tests that previously asserted the existence of bugs to now confirm their resolution, ensuring that the test suite accurately reflects the current state of the codebase. Furthermore, it introduces alias support for configuration sections, enhancing user experience and compatibility between different CLI tools, while also providing a detailed assessment of remaining configuration discrepancies. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 is an excellent pull request that provides a thorough assessment of the configuration system and updates the integration tests to reflect recent bug fixes. The new markdown report is very clear and will be a great asset for tracking progress. The test updates are also well-executed, correctly flipping assertions from documenting bugs to verifying fixes. I have one suggestion to strengthen a test that was weakened during the refactoring, but overall this is a high-quality contribution.
| def test_eval_batch_size_has_proper_fallback(self): | ||
| """train.py uses a proper fallback for eval_batch_size.""" | ||
| tree = self._parse_train_py() | ||
| kwargs = self._extract_train_model_kwargs(tree) | ||
| assert "per_device_eval_batch_size" in kwargs |
There was a problem hiding this comment.
This test was weakened from its original version, which checked the contents of the fallback logic. The new version only asserts that the per_device_eval_batch_size argument is present.
To ensure the test properly verifies the fallback logic as the docstring suggests, we can inspect the AST for the eval_batch_size assignment and confirm that it includes both eval_batch_size (the preferred option) and train_batch_size (the final fallback).
def test_eval_batch_size_has_proper_fallback(self):
"""train.py uses a proper fallback for eval_batch_size."""
tree = self._parse_train_py()
kwargs = self._extract_train_model_kwargs(tree)
assert "per_device_eval_batch_size" in kwargs
# Find the assignment to `eval_batch_size` in the AST
assign_node = next(
(n for n in ast.walk(tree) if isinstance(n, ast.Assign) and
any(isinstance(t, ast.Name) and t.id == 'eval_batch_size' for t in n.targets)),
None
)
assert assign_node is not None, "Could not find assignment to `eval_batch_size`"
# Check that the fallback logic includes both eval_batch_size and train_batch_size
source_dump = ast.dump(assign_node.value)
assert "eval_batch_size" in source_dump, (
"Fallback logic should prioritize 'eval_batch_size' from config"
)
assert "train_batch_size" in source_dump, (
"Fallback logic should use 'train_batch_size' as a final fallback"
)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces a comprehensive assessment document analyzing GLiNER's configuration system compatibility and updates integration tests to reflect recently implemented features: configuration section aliasing across CLIs, lazy import behavior, and enhanced field forwarding between components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Review NotesThis PR updates stale "bug documentation" tests to assert fixed behavior:
The new
The PR appropriately maintains test accuracy by updating assertions to match the current fixed state of the codebase. |
![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
🧹 Nitpick comments (1)
tests/test_validator_integration.py (1)
518-528: Make the forwarding-gap assertion strict to catch regressions.Using
issubsetallows newly-missing fields to slip by unnoticed. A strict equality check will preserve the test’s intent (“only these remain as gaps”).✅ Suggested tightening
- assert expected_missing.issubset(actual_missing), ( - f"Expected these config fields to be missing from train.py forwarding: " - f"{expected_missing}. Actually missing: {actual_missing}" - ) - # Confirm label_smoothing is no longer in the gaps - assert "label_smoothing" not in actual_missing, ( - "label_smoothing should now be forwarded by train.py" - ) + assert actual_missing == expected_missing, ( + "Forwarding gaps changed unexpectedly. " + f"Expected: {expected_missing}. Actual: {actual_missing}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_validator_integration.py` around lines 518 - 528, Replace the non-strict subset check with a strict equality check so the test fails if any additional fields are missing; specifically, compare expected_missing to actual_missing using equality (use the variables expected_missing and actual_missing derived from not_forwarded) instead of calling expected_missing.issubset(actual_missing), and keep the existing assertion messages but update them to reflect equality semantics (confirming these are the only missing train.py forwarding fields and that "label_smoothing" is not present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@research/gliner_config_assessment.md`:
- Around line 27-35: Update the "Test Results Summary" block in
research/gliner_config_assessment.md to indicate these results are historical by
adding a baseline reference (date and/or commit SHA) and/or updating the test
counts to current CI; specifically modify the Test Results Summary table header
or a single-line note underneath (near the "Test Results Summary" title) to
mention the baseline (e.g., "Results as of YYYY-MM-DD / commit <SHA>") so
readers know the `ptbr/tests/test_config_cli.py`,
`ptbr/tests/test_config_cli_aliases.py`, `ptbr/tests/test_training_cli.py`, and
`tests/test_validator_integration.py` counts are not necessarily current.
In `@tests/test_validator_integration.py`:
- Around line 544-555: The test currently hardcodes output_dir="/tmp/_test_ruc"
when constructing TrainingArguments; replace this with a temporary directory
(e.g., use tempfile.TemporaryDirectory() or the pytest tmp_path/tmp_path_factory
fixture) so the test is portable and secure. Update the try block that creates
TrainingArguments (the line with args =
TrainingArguments(output_dir="/tmp/_test_ruc")) to create and pass a temp dir
path, ensure the temp directory is cleaned up (use context manager or fixture
scope), and adjust imports to add tempfile or rely on the pytest tmp_path
fixture.
---
Nitpick comments:
In `@tests/test_validator_integration.py`:
- Around line 518-528: Replace the non-strict subset check with a strict
equality check so the test fails if any additional fields are missing;
specifically, compare expected_missing to actual_missing using equality (use the
variables expected_missing and actual_missing derived from not_forwarded)
instead of calling expected_missing.issubset(actual_missing), and keep the
existing assertion messages but update them to reflect equality semantics
(confirming these are the only missing train.py forwarding fields and that
"label_smoothing" is not present).
| ### Test Results Summary | ||
|
|
||
| | Test Suite | Result | Notes | | ||
| |-----------|--------|-------| | ||
| | `ptbr/tests/test_config_cli.py` | **Cannot run** | Imports `gliner.config.GLiNERConfig` which triggers `gliner/__init__.py` → `gliner/model.py` → `onnxruntime` (not installed). The test file lacks a mock strategy for heavy DL imports. | | ||
| | `ptbr/tests/test_config_cli_aliases.py` | **3/3 passed** | Uses monkeypatch + fake `GLiNERConfig` stub. Confirms `model:` alias and `lora:` alias work correctly. | | ||
| | `ptbr/tests/test_training_cli.py` | **62/62 passed** | Comprehensive. Covers `_deep_get`/`_deep_set`, `_check_type`, `validate_config`, `semantic_checks`, `check_huggingface`, `check_wandb`, `check_resume`, CLI integration, edge cases, LoRA application, and training parameter forwarding. | | ||
| | `tests/test_validator_integration.py` | **22 passed, 25 failed** | See detailed analysis below. | | ||
|
|
There was a problem hiding this comment.
Clarify that these test results are historical to avoid contradicting current CI.
The summary still reports failing suites even though the PR updates those tests. Add a baseline date/commit (or update counts) so readers don’t assume these results reflect the current state.
📝 Suggested clarification
-### Test Results Summary
+### Test Results Summary (baseline run — 2026-02-18, pre-test updates)
@@
-| `tests/test_validator_integration.py` | **22 passed, 25 failed** | See detailed analysis below. |
+| `tests/test_validator_integration.py` | **22 passed, 25 failed** | Baseline before stale-test updates; current CI should be green. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@research/gliner_config_assessment.md` around lines 27 - 35, Update the "Test
Results Summary" block in research/gliner_config_assessment.md to indicate these
results are historical by adding a baseline reference (date and/or commit SHA)
and/or updating the test counts to current CI; specifically modify the Test
Results Summary table header or a single-line note underneath (near the "Test
Results Summary" title) to mention the baseline (e.g., "Results as of YYYY-MM-DD
/ commit <SHA>") so readers know the `ptbr/tests/test_config_cli.py`,
`ptbr/tests/test_config_cli_aliases.py`, `ptbr/tests/test_training_cli.py`, and
`tests/test_validator_integration.py` counts are not necessarily current.
| try: | ||
| args = TrainingArguments(output_dir="/tmp/_test_ruc") | ||
| except ImportError: | ||
| # accelerate not installed — check field default via dataclass inspection | ||
| import dataclasses | ||
| for f in dataclasses.fields(TrainingArguments): | ||
| if f.name == "remove_unused_columns": | ||
| assert f.default is True, ( | ||
| "HF TrainingArguments defaults remove_unused_columns to True" | ||
| ) | ||
| return | ||
| pytest.fail("TrainingArguments has no remove_unused_columns field") |
There was a problem hiding this comment.
Avoid hardcoded /tmp path; use a temporary directory for portability/security.
Ruff flagged this and it also avoids cross-platform issues.
🔧 Safer temp directory usage
- try:
- args = TrainingArguments(output_dir="/tmp/_test_ruc")
- except ImportError:
+ import tempfile
+ try:
+ with tempfile.TemporaryDirectory() as tmpdir:
+ args = TrainingArguments(output_dir=tmpdir)
+ except ImportError:🧰 Tools
🪛 Ruff (0.15.1)
[error] 545-545: Probable insecure usage of temporary file or directory: "/tmp/_test_ruc"
(S108)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_validator_integration.py` around lines 544 - 555, The test
currently hardcodes output_dir="/tmp/_test_ruc" when constructing
TrainingArguments; replace this with a temporary directory (e.g., use
tempfile.TemporaryDirectory() or the pytest tmp_path/tmp_path_factory fixture)
so the test is portable and secure. Update the try block that creates
TrainingArguments (the line with args =
TrainingArguments(output_dir="/tmp/_test_ruc")) to create and pass a temp dir
path, ensure the temp directory is cleaned up (use context manager or fixture
scope), and adjust imports to add tempfile or rely on the pytest tmp_path
fixture.
- Merge latest dev (47 commits) to bring branch up to date
- Resolve merge conflicts in tests/test_validator_integration.py (take dev version)
- Cherry-pick review fixes from claude/fix-failing-tests-arelM:
- Fix _get_train_model_kwarg_names() to handle **{...} dict-expansion kwargs
- Replace hardcoded /tmp path with tempfile.mkdtemp() (S108)
- Fix label_smoothing None-safety in train.py for YAML null values
- Set both eval_strategy and evaluation_strategy for transformers v4/v5
- Fix unused unpacked variables across test files (RUF059)
- Fix unused **kwargs -> **_kwargs in mock helpers (ARG001)
- Fix == True/False comparisons -> truthiness checks (E712)
- Remove redundant inner Mock imports (F811)
https://claude.ai/code/session_01E2vyUyqeJ2hNWxFR3mFzNP
2 participants
The integration tests in test_validator_integration.py were written as "bug documentation" tests that asserted bugs exist. Several bugs were fixed in the codebase but the tests were never updated, causing 25 failures. This commit:
All 195 tests pass (81+3+62+49).
https://claude.ai/code/session_018GMATjW6TCi7KbKUqGE9Uk