Add ptbr data preparation module with typer CLI - #31
Conversation
- Fix TestSchemaVsForwarding: dead config fields (size_sup, shuffle_types, random_drop) were removed from _FIELD_SCHEMA, so update test to assert zero forwarding gaps instead of expecting them as gaps - Add test_dead_config_fields_removed_from_schema to verify removal - Rename duplicate test_run_name_forwarded to test_run_name_forwarded_to_training_args - Update integration report with current test results (49/49 integration, 176/176 ptbr), CLI usage instructions, and status corrections - Add 3 example YAML configs: basic NER, LoRA fine-tuning, token-level NER All tests pass: 49/49 integration + 176/176 ptbr. https://claude.ai/code/session_01H4dfwAPTYACKB2VkXfyoJ2
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Sorry @arthrod, your pull request is larger than the review limit of 150000 diff characters
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR standardizes token-level naming conventions (token-level → token_level), introduces comprehensive trust_remote_code parameter propagation through model loading paths, expands TrainingArguments with new hyperparameters (label_smoothing, eval_steps, seed, push_to_hub, etc.), and adds a complete ptbr CLI toolkit with configuration validation, data handling, and training orchestration including LoRA support and external service integration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 unit tests (beta)
Comment |
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 introduces a new command-line interface ( 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new ptbr module with a Typer-based CLI for data preparation, along with several example configuration files. The changes also include significant updates to the integration report, reflecting numerous fixes and adding comprehensive CLI usage documentation. The test suite has been improved to validate these fixes, such as the removal of dead configuration fields.
My review identifies two medium-severity maintainability issues: one concerning the duplication of example configuration files within the integration report, and another regarding a duplicated test method in the integration tests. Addressing these will improve the long-term maintainability of the documentation and test suite. Overall, this is a great addition that improves the usability and robustness of the project's training workflow.
| ### Appendix: Example YAML Configurations | ||
|
|
||
| #### Example 1: Basic NER Training (`examples/config_ner_basic.yaml`) | ||
|
|
||
| ```yaml | ||
| run: | ||
| name: "gliner-ner-basic" | ||
| description: "Basic English NER fine-tuning with DeBERTa-v3-small" | ||
| tags: ["ner", "english", "span"] | ||
| seed: 42 | ||
|
|
||
| model: | ||
| model_name: "microsoft/deberta-v3-small" | ||
| name: "gliner-ner-basic" | ||
| span_mode: "markerV0" | ||
| max_width: 12 | ||
| hidden_size: 768 | ||
| dropout: 0.3 | ||
| fine_tune: true | ||
| subtoken_pooling: "first" | ||
| max_len: 384 | ||
| max_types: 25 | ||
| max_neg_type_ratio: 1 | ||
|
|
||
| data: | ||
| root_dir: "logs/ner_basic" | ||
| train_data: "data/train.json" | ||
| val_data_dir: "none" | ||
|
|
||
| training: | ||
| num_steps: 10000 | ||
| train_batch_size: 8 | ||
| eval_every: 500 | ||
| warmup_ratio: 0.1 | ||
| scheduler_type: "cosine" | ||
| lr_encoder: 1.0e-5 | ||
| lr_others: 3.0e-5 | ||
| weight_decay_encoder: 0.01 | ||
| weight_decay_other: 0.01 | ||
| max_grad_norm: 10.0 | ||
| optimizer: "adamw_torch" | ||
| loss_alpha: -1 | ||
| loss_gamma: 0 | ||
| label_smoothing: 0 | ||
| loss_reduction: "sum" | ||
| bf16: false | ||
| save_total_limit: 3 | ||
| dataloader_num_workers: 2 | ||
|
|
||
| lora: | ||
| enabled: false | ||
|
|
||
| environment: | ||
| push_to_hub: false | ||
| report_to: "none" | ||
| ``` | ||
|
|
||
| #### Example 2: LoRA Fine-Tuning (`examples/config_ner_lora.yaml`) | ||
|
|
||
| ```yaml | ||
| run: | ||
| name: "gliner-ner-lora-finetune" | ||
| description: "LoRA fine-tuning for domain-specific NER" | ||
| tags: ["ner", "lora", "efficient"] | ||
| seed: 123 | ||
|
|
||
| model: | ||
| model_name: "microsoft/deberta-v3-base" | ||
| name: "gliner-ner-lora" | ||
| span_mode: "markerV0" | ||
| max_width: 12 | ||
| hidden_size: 768 | ||
| dropout: 0.4 | ||
| fine_tune: true | ||
| subtoken_pooling: "first" | ||
| max_len: 512 | ||
| max_types: 25 | ||
| max_neg_type_ratio: 1 | ||
|
|
||
| data: | ||
| root_dir: "logs/ner_lora" | ||
| train_data: "data/train.json" | ||
| val_data_dir: "data/val.json" | ||
|
|
||
| training: | ||
| num_steps: 5000 | ||
| train_batch_size: 4 | ||
| eval_every: 250 | ||
| warmup_ratio: 0.05 | ||
| scheduler_type: "linear" | ||
| lr_encoder: 5.0e-5 | ||
| lr_others: 1.0e-4 | ||
| weight_decay_encoder: 0.01 | ||
| weight_decay_other: 0.01 | ||
| max_grad_norm: 1.0 | ||
| optimizer: "adamw_torch" | ||
| loss_alpha: 0.75 | ||
| loss_gamma: 2.0 | ||
| label_smoothing: 0.1 | ||
| loss_reduction: "sum" | ||
| bf16: true | ||
| save_total_limit: 2 | ||
| gradient_accumulation_steps: 4 | ||
| dataloader_num_workers: 4 | ||
| dataloader_pin_memory: true | ||
|
|
||
| lora: | ||
| enabled: true | ||
| r: 16 | ||
| lora_alpha: 32 | ||
| lora_dropout: 0.05 | ||
| bias: "none" | ||
| target_modules: ["q_proj", "v_proj"] | ||
| task_type: "TOKEN_CLS" | ||
|
|
||
| environment: | ||
| push_to_hub: false | ||
| report_to: "wandb" | ||
| wandb_project: "gliner-lora-experiments" | ||
| ``` | ||
|
|
||
| #### Example 3: Token-Level NER (`examples/config_token_level.yaml`) | ||
|
|
||
| ```yaml | ||
| run: | ||
| name: "gliner-token-level-ner" | ||
| description: "Token-level NER using sequence labeling" | ||
| tags: ["ner", "token-level", "conll"] | ||
| seed: 7 | ||
|
|
||
| model: | ||
| model_name: "microsoft/deberta-v3-small" | ||
| name: "gliner-token-ner" | ||
| span_mode: "token_level" | ||
| max_width: 12 | ||
| hidden_size: 512 | ||
| dropout: 0.3 | ||
| fine_tune: true | ||
| subtoken_pooling: "first" | ||
| max_len: 256 | ||
| max_types: 50 | ||
| max_neg_type_ratio: 1 | ||
| num_rnn_layers: 1 | ||
|
|
||
| data: | ||
| root_dir: "logs/token_ner" | ||
| train_data: "data/train.json" | ||
| val_data_dir: "data/val.json" | ||
|
|
||
| training: | ||
| num_steps: 20000 | ||
| train_batch_size: 16 | ||
| eval_every: 1000 | ||
| warmup_ratio: 0.1 | ||
| scheduler_type: "cosine" | ||
| lr_encoder: 2.0e-5 | ||
| lr_others: 5.0e-5 | ||
| weight_decay_encoder: 0.01 | ||
| weight_decay_other: 0.01 | ||
| max_grad_norm: 5.0 | ||
| optimizer: "adamw_torch" | ||
| loss_alpha: -1 | ||
| loss_gamma: 0 | ||
| label_smoothing: 0 | ||
| loss_reduction: "mean" | ||
| bf16: false | ||
| fp16: true | ||
| save_total_limit: 5 | ||
| dataloader_num_workers: 2 | ||
| dataloader_pin_memory: true | ||
| dataloader_persistent_workers: true | ||
| dataloader_prefetch_factor: 4 | ||
|
|
||
| lora: | ||
| enabled: false | ||
|
|
||
| environment: | ||
| push_to_hub: true | ||
| hub_model_id: "my-org/gliner-token-ner-v1" | ||
| report_to: "none" | ||
| ``` |
There was a problem hiding this comment.
The full content of the example YAML files from the examples/ directory is duplicated in this appendix. This creates a maintainability issue, as any updates to the source YAML files will require manual updates here to prevent the documentation from becoming outdated and misleading. This is highlighted by the fact that the YAML format for lists (like tags and target_modules) is already inconsistent between the source files and this documentation (multi-line vs. single-line array format).
It would be better to remove these duplicated YAML blocks and simply refer to the example files (e.g., examples/config_ner_basic.yaml). This ensures the documentation always points to the single source of truth.
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/configs.md (2)
333-333:⚠️ Potential issue | 🟡 MinorYAML example uses outdated
token-levelvalue.The narrative text (lines 303-305) correctly states
span_modeis fixed to"token_level", but this YAML example still uses"token-level". This inconsistency will cause configuration errors.# Model Configuration model_name: microsoft/deberta-v3-base labels_encoder: null name: "token level gliner" hidden_size: 768 dropout: 0.4 fine_tune: true subtoken_pooling: first -span_mode: token-level # Token-level prediction +span_mode: token_level # Token-level prediction num_rnn_layers: 1 # LSTM helps with token sequences🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/configs.md` at line 333, The YAML example uses the old value "token-level" for span_mode which is inconsistent with the documented enum; update the example so span_mode uses the canonical value token_level (i.e., replace "token-level" with token_level) to match the narrative and the code that expects the span_mode symbol token_level.
514-514:⚠️ Potential issue | 🟡 MinorYAML example uses outdated
token-levelvalue.Same inconsistency as above—the BiEncoder Token configuration example uses
"token-level"instead of"token_level".# Model Configuration model_name: microsoft/deberta-v3-base labels_encoder: sentence-transformers/all-MiniLM-L6-v2 name: "bi-encoder token gliner" hidden_size: 768 dropout: 0.4 fine_tune: true subtoken_pooling: first -span_mode: token-level +span_mode: token_level num_rnn_layers: 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/configs.md` at line 514, The YAML example for the BiEncoder Token configuration uses the wrong value for span_mode; update the value from "token-level" to the canonical "token_level" (i.e., change span_mode: token-level to span_mode: token_level) so the example matches the expected configuration format for the BiEncoder Token.
🧹 Nitpick comments (10)
pyproject.toml (1)
15-15: Update Rufftarget-versiontopy311.
Currentlytarget-version = "py39"misaligns withrequires-python = ">=3.11". Set it topy311so linting rules match the supported Python baseline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 15, The Ruff configuration's target-version in pyproject.toml currently specifies "py39" which conflicts with requires-python = ">=3.11"; update the ruff setting target-version to "py311" so linting rules align with the project's Python baseline (change the target-version key in pyproject.toml from "py39" to "py311").tests/test_modeling.py (6)
1050-1051: Remove redundantMockimport.Same issue as above—
Mockis already imported at module level.def test_forward_with_precomputed_labels_embeds(self, mock_config, model_inputs): """Should accept precomputed labels embeddings instead of ids.""" - from unittest.mock import Mock🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_modeling.py` around lines 1050 - 1051, Remove the redundant "from unittest.mock import Mock" local import in the test (the symbol Mock is already imported at module scope); simply delete this duplicate import line so the test file uses the existing module-level Mock import and avoids an unnecessary duplicate import.
1027-1028: Remove redundantMockimport.
Mockis already imported at line 9 fromunittest.mock. This local import shadows the module-level import and triggers a redefinition warning.def test_forward_output_shape_without_labels(self, mock_config, model_inputs): """Should return output with correct logits shape without labels.""" - from unittest.mock import Mock # Remove labels to test inference mode🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_modeling.py` around lines 1027 - 1028, Remove the redundant local import "from unittest.mock import Mock" (which shadows the module-level Mock imported earlier) by deleting that line in tests/test_modeling.py so the file uses the existing top-level Mock import; ensure no other local re-imports of Mock remain and run tests to confirm nothing else depended on the duplicated import.
481-481: Use bitwise negation for tensor boolean comparison.- assert torch.all(pair_mask == False) + assert torch.all(~pair_mask)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_modeling.py` at line 481, The test currently compares the boolean tensor pair_mask with False using equality (assert torch.all(pair_mask == False)), which is not idiomatic — replace that comparison with a boolean negation of the tensor (e.g., use torch.all(~pair_mask) or torch.all(torch.logical_not(pair_mask))) so the assertion directly negates pair_mask; update the assertion referencing the pair_mask variable and keep the torch.all(...) wrapper.
323-324: Use direct boolean checks instead of equality comparisons.- assert word_mask[0, 0] == True - assert word_mask[0, 1] == True + assert word_mask[0, 0] + assert word_mask[0, 1]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_modeling.py` around lines 323 - 324, Replace the equality comparisons against True in the test assertions with direct boolean checks: update the two assertions that reference word_mask (word_mask[0, 0] and word_mask[0, 1]) so they assert the truthiness directly (e.g., assert word_mask[0, 0] and assert word_mask[0, 1]) rather than using == True.
90-96: Use identity checks instead of equality comparisons for booleans.Static analysis flags these equality comparisons. Use direct boolean evaluation instead:
- assert mask[0, 0] == True - assert mask[0, 1] == True + assert mask[0, 0] + assert mask[0, 1] # ... - assert mask[1, 0] == True + assert mask[1, 0] if basic_setup["max_text_length"] > 1: - assert mask[1, 1] == False + assert not mask[1, 1]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_modeling.py` around lines 90 - 96, Replace boolean equality assertions in tests with direct boolean evaluations: instead of "assert mask[0, 0] == True" and "assert mask[0, 1] == True" use direct truthy assertions for mask[0,0] and mask[0,1], and for the second batch replace "assert mask[1, 0] == True" with a direct truthy assertion for mask[1,0]; similarly replace the conditional "assert mask[1, 1] == False" with a direct falsy assertion (e.g., using "not") for mask[1,1]. Locate these checks on the mask variable in tests/test_modeling.py and update them to use direct boolean evaluation.
123-123: Usenot maskinstead of== False.- assert torch.all(mask == False) + assert torch.all(~mask)Or alternatively use
assert not mask.any()for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_modeling.py` at line 123, Replace the equality check against False with a direct negation: change the assertion that currently uses torch.all(mask == False) to use a boolean negation on the mask (e.g., assert not mask or assert not mask.any()) so the intent is clearer and avoids tensor == False; update the assertion referencing the mask variable and torch.all accordingly.tests/test_trainer_column_pruning.py (2)
8-13: String-based method extraction is brittle.The
_method_blockfunction relies on string splitting to extract method bodies. This approach is fragile:
- Assumes methods appear in a specific order
- Breaks if
next_method_nameappears in comments or strings before the actual method- Fails silently if method names change
Consider using AST-based extraction for robustness:
♻️ Suggested improvement using AST
import ast def _method_block(source: str, method_name: str) -> str: """Extract method body using AST parsing.""" tree = ast.parse(source) for node in ast.walk(tree): if isinstance(node, ast.FunctionDef) and node.name == method_name: return ast.get_source_segment(source, node) raise ValueError(f"Method {method_name} not found")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_trainer_column_pruning.py` around lines 8 - 13, The _method_block helper is brittle because it extracts method text via string splitting; replace it with an AST-based extraction to robustly locate FunctionDef nodes by name (use ast.parse and ast.walk to find the node whose .name matches the requested method and then ast.get_source_segment to return the exact source text), remove reliance on next_method_name ordering, and raise a clear ValueError if the method is not found; update callers of _method_block accordingly to handle the new signature/exception.
28-30: Test assertion is overly specific.The assertion
"trainer = Trainer(**trainer_kwargs)" in sourcewill fail if:
- Spacing changes (e.g.,
trainer=Trainer(...))- Variable name changes
- Additional arguments are added inline
Consider a more flexible pattern match or testing actual behavior instead of source strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_trainer_column_pruning.py` around lines 28 - 30, The test test_train_model_uses_custom_gliner_trainer is too rigid by checking the exact source substring "trainer = Trainer(**trainer_kwargs)"; update it to robustly detect a Trainer instantiation instead—either use a regex like r"trainer\s*=\s*Trainer\s*\(" on MODEL_SOURCE.read_text(...) to allow spacing/arg changes, or parse the source with ast and assert there is an Assign to a target named "trainer" whose value is a Call to a Name "Trainer"; change the assertion to use that regex or AST check so variable/spacing/inline-args variations won't break the test.tests/test_config_propagation.py (1)
65-92: Consider stability of internal PyTorch module usage.
torch._subclasses.fake_tensoris an internal/private module (indicated by the leading underscore). While it's useful for testing, internal APIs may change without notice across PyTorch versions. The test currently validates bf16 dtype handling correctly, but be aware this may require updates with future PyTorch releases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_config_propagation.py` around lines 65 - 92, The test imports the private module torch._subclasses.fake_tensor which can break across PyTorch versions; modify the test_fake_tensor_cpu_path_reflects_bf16_training_arg to avoid relying on that internal module by either importing a stable public API if available (e.g., prefer torch.testing utilities) or by wrapping the private import in a try/except and skipping the test when unavailable, keeping the rest of the test (calls to BaseGLiNER.create_training_args and the dtype/device assertions) unchanged so behavior is validated only when a supported fake-tensor API exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ptbr/config_cli.py`:
- Around line 184-211: In _coerce_type, the int coercion silently truncates
non‑integral values (e.g. int(1.9) -> 1); update the expected_type is int branch
to reject non‑integral floats/strings: for float inputs, allow only values where
value.is_integer() is True (otherwise raise TypeError); for string inputs, try
parsing as int first, and if that fails try parsing as float and accept only if
that float is integral (value.is_integer()), otherwise raise TypeError; keep the
existing bool rejection and return int(value) only when the value is
demonstrably an integer.
In `@ptbr/template.yaml`:
- Around line 309-311: The documentation comment for the max_grad_norm setting
is inconsistent with its value; update the comment to match the actual value or
change the value to the intended default—specifically, either change the comment
to "Default: 10.0" to match max_grad_norm: 10.0, or set max_grad_norm to 1.0 if
the intended default is 1.0 so the code and comment agree.
- Around line 147-152: The comment for the YAML key hidden_size is inconsistent
with its value—update either the comment or the value so they match: locate the
hidden_size entry in template.yaml and either change the inline comment
"Default: 512" to the actual default "Default: 768" if 768 is intended, or
revert the value from 768 to 512 if the intended default is 512; ensure the
dropout comment/value pair remains unchanged and commit the matching comment and
value together.
In `@ptbr/tests/test_config_cli.py`:
- Around line 165-185: Tests assign the return of _validate_section to the
unused variable result in test_default_fields_produce_warnings and
test_fully_specified_no_warnings, causing Ruff F841; remove the unused
assignment or replace result with _ (e.g., call _validate_section(data,
_GLINER_RULES, "gliner_config", report) without assigning) so the function is
still invoked but no unused variable remains; update both occurrences
referencing the variable name result in those tests.
In `@ptbr/tests/test_train_py.py`:
- Around line 21-22: Ruff flags the unused parameter in the _DummyModel.to
method; rename the parameter to _dtype (or prefix with an underscore) to mark it
intentionally unused. Update the function signature def to(self, _dtype=None):
return self so the linter no longer reports ARG002 while preserving behavior;
reference the to method and the dtype parameter when making the change.
In `@ptbr/tests/test_training_cli.py`:
- Around line 224-373: Ruff is flagging test-only token literals (S105);
suppress these false positives by adding a local ruff noqa for S105 on the lines
that set dummy tokens/constants used in tests (e.g., the assignments in
test_sensitive_values_are_redacted where "hf_secret_token"/"wandb_secret_key"
are set, and in TestCheckHuggingFace tests: test_error_no_token,
test_success_with_mock, test_failure_status, test_network_error where
"hf_token"/"hf_testtoken"/"hf_badtoken"/"hf_token" are used). Add an inline
comment "# noqa: S105" (or the equivalent ruff suppression) next to each
dummy-token assignment so linter warnings are silenced while keeping the test
logic unchanged.
- Around line 539-542: The test function test_validate_writes_summary currently
declares an unused fixture parameter tmp_path; remove tmp_path from the function
signature so it becomes def test_validate_writes_summary(self, cfg_file: Path)
-> None (or alternatively use tmp_path if intended), ensuring the unique test
function name test_validate_writes_summary is updated accordingly to eliminate
the ARG002 unused-argument lint failure.
In `@ptbr/tests/test_validation.py`:
- Around line 331-339: The linter flags the subprocess.run call in
_run_validate_cli with S603 even though we invoke it without a shell and the
path is test-controlled; suppress this specific warning locally by adding an
inline Ruff/flake8 noqa for S603 on the subprocess.run invocation (the call
within function _run_validate_cli) so the test stays lint-clean while keeping
shell=False and stdout/stderr redirects intact.
In `@ptbr/training_cli.py`:
- Around line 425-449: The helper _check_extra_keys currently recurses into
every dict and thus flags nested keys under free-form dict leaves like
model.encoder_config; modify the dict branch to detect when the current full key
is a known schema leaf (i.e., full is in known but there are no known keys that
start with full + ".") and in that case do NOT recurse (treat the whole dict as
an allowed free-form leaf), otherwise continue to recurse; update
_check_extra_keys to compute whether any known key startswith(f"{full}.") and
skip recursion when full in known and no such child keys exist.
- Around line 1181-1218: The task_map in _apply_lora() omits the
FEATURE_EXTRACTION mapping so lora_cfg["task_type"] == "FEATURE_EXTRACTION"
silently falls back to TOKEN_CLS; update task_map to include
"FEATURE_EXTRACTION": TaskType.FEATURE_EXTRACTION (or the correct enum value
supported by PEFT) and use that when constructing the LoraConfig; additionally,
if TaskType.FEATURE_EXTRACTION is not supported by the installed PEFT version,
detect that and emit a clear logger.warning in _apply_lora() explaining the
fallback to TaskType.TOKEN_CLS before constructing LoraConfig so the user is
informed of the change in behavior.
In `@tests/test_config_validation.py`:
- Around line 473-505: Replace the mutable dict assigned to the
TestDefaultValues class attribute UPSTREAM_BASE_DEFAULTS with an immutable
mapping (use types.MappingProxyType wrapping the literal dict) and add the
required import from types; likewise convert any other mutable class-attribute
defaults in this tests module to immutable structures (MappingProxyType for
mappings or tuples for lists) so class constants are truly immutable.
- Around line 531-538: Ruff flags the string literal "<<REL>>" in
test_relex_defaults as a potential secret (S105); suppress this false positive
by adding a local noqa for S105 on the assertion or the variable use in the
test: update test_relex_defaults (the UniEncoderRelexConfig instantiation and
the assertion checking cfg.rel_token == "<<REL>>") to include an inline
suppression comment such as "# noqa: S105" immediately after the literal or
assertion so only this line is ignored.
In `@tests/test_decoder.py`:
- Around line 533-542: The test `test_no_relations_when_not_requested` unpacks
the result of `SpanRelexDecoder.decode` into `spans, relations` but never uses
`spans`, triggering Ruff RUF059; change the unused variable to `_` (or `_spans`)
in that test so the unpack becomes `_, relations = decoder.decode(...)` to
satisfy the linter while leaving `SpanRelexDecoder.decode` behavior unchanged.
In `@tests/test_training_validation.py`:
- Around line 165-175: The class-level constant set REQUIRED_GLINER_FIELDS is
mutable and should be frozen to satisfy RUF012; replace its definition with a
frozenset (e.g., frozenset({...})) to make it immutable and do the same for the
other mutable class-level sets referenced around lines ~580-590 (identify their
names in the file and convert them to frozenset as well), ensuring you update
any tests or usages that rely on set-specific mutating methods if present.
In `@tests/test_validator_integration.py`:
- Around line 474-479: Replace the mutable set KNOWN_NON_FORWARDED with an
immutable collection: change the current set literal assigned to
KNOWN_NON_FORWARDED to a frozenset (or tuple) to satisfy the linter; update the
declaration where KNOWN_NON_FORWARDED is defined so that any references to
KNOWN_NON_FORWARDED elsewhere (e.g., tests expecting membership checks) continue
to work unchanged.
- Around line 813-820: CONFIG_FILES is defined as a mutable list; change it to
an immutable tuple to satisfy Ruff RUF012. Replace the list literal assigned to
CONFIG_FILES with a tuple literal (e.g., ("config.yaml", "config_span.yaml",
...)) so the constant is immutable while keeping the same values; ensure any
code that iterates over CONFIG_FILES continues to work (tuples support
iteration).
- Around line 544-549: The test test_default_remove_unused_columns_is_true uses
a hardcoded path "/tmp/_test_ruc"; update the test to accept the pytest tmp_path
fixture and instantiate TrainingArguments with a temporary directory (e.g.,
str(tmp_path / "_test_ruc")) instead of the literal "/tmp/_test_ruc" so the test
is portable and lint-clean; locate the test function
test_default_remove_unused_columns_is_true and replace the
TrainingArguments(output_dir=...) argument accordingly.
---
Outside diff comments:
In `@docs/configs.md`:
- Line 333: The YAML example uses the old value "token-level" for span_mode
which is inconsistent with the documented enum; update the example so span_mode
uses the canonical value token_level (i.e., replace "token-level" with
token_level) to match the narrative and the code that expects the span_mode
symbol token_level.
- Line 514: The YAML example for the BiEncoder Token configuration uses the
wrong value for span_mode; update the value from "token-level" to the canonical
"token_level" (i.e., change span_mode: token-level to span_mode: token_level) so
the example matches the expected configuration format for the BiEncoder Token.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 15: The Ruff configuration's target-version in pyproject.toml currently
specifies "py39" which conflicts with requires-python = ">=3.11"; update the
ruff setting target-version to "py311" so linting rules align with the project's
Python baseline (change the target-version key in pyproject.toml from "py39" to
"py311").
In `@tests/test_config_propagation.py`:
- Around line 65-92: The test imports the private module
torch._subclasses.fake_tensor which can break across PyTorch versions; modify
the test_fake_tensor_cpu_path_reflects_bf16_training_arg to avoid relying on
that internal module by either importing a stable public API if available (e.g.,
prefer torch.testing utilities) or by wrapping the private import in a
try/except and skipping the test when unavailable, keeping the rest of the test
(calls to BaseGLiNER.create_training_args and the dtype/device assertions)
unchanged so behavior is validated only when a supported fake-tensor API exists.
In `@tests/test_modeling.py`:
- Around line 1050-1051: Remove the redundant "from unittest.mock import Mock"
local import in the test (the symbol Mock is already imported at module scope);
simply delete this duplicate import line so the test file uses the existing
module-level Mock import and avoids an unnecessary duplicate import.
- Around line 1027-1028: Remove the redundant local import "from unittest.mock
import Mock" (which shadows the module-level Mock imported earlier) by deleting
that line in tests/test_modeling.py so the file uses the existing top-level Mock
import; ensure no other local re-imports of Mock remain and run tests to confirm
nothing else depended on the duplicated import.
- Line 481: The test currently compares the boolean tensor pair_mask with False
using equality (assert torch.all(pair_mask == False)), which is not idiomatic —
replace that comparison with a boolean negation of the tensor (e.g., use
torch.all(~pair_mask) or torch.all(torch.logical_not(pair_mask))) so the
assertion directly negates pair_mask; update the assertion referencing the
pair_mask variable and keep the torch.all(...) wrapper.
- Around line 323-324: Replace the equality comparisons against True in the test
assertions with direct boolean checks: update the two assertions that reference
word_mask (word_mask[0, 0] and word_mask[0, 1]) so they assert the truthiness
directly (e.g., assert word_mask[0, 0] and assert word_mask[0, 1]) rather than
using == True.
- Around line 90-96: Replace boolean equality assertions in tests with direct
boolean evaluations: instead of "assert mask[0, 0] == True" and "assert mask[0,
1] == True" use direct truthy assertions for mask[0,0] and mask[0,1], and for
the second batch replace "assert mask[1, 0] == True" with a direct truthy
assertion for mask[1,0]; similarly replace the conditional "assert mask[1, 1] ==
False" with a direct falsy assertion (e.g., using "not") for mask[1,1]. Locate
these checks on the mask variable in tests/test_modeling.py and update them to
use direct boolean evaluation.
- Line 123: Replace the equality check against False with a direct negation:
change the assertion that currently uses torch.all(mask == False) to use a
boolean negation on the mask (e.g., assert not mask or assert not mask.any()) so
the intent is clearer and avoids tensor == False; update the assertion
referencing the mask variable and torch.all accordingly.
In `@tests/test_trainer_column_pruning.py`:
- Around line 8-13: The _method_block helper is brittle because it extracts
method text via string splitting; replace it with an AST-based extraction to
robustly locate FunctionDef nodes by name (use ast.parse and ast.walk to find
the node whose .name matches the requested method and then
ast.get_source_segment to return the exact source text), remove reliance on
next_method_name ordering, and raise a clear ValueError if the method is not
found; update callers of _method_block accordingly to handle the new
signature/exception.
- Around line 28-30: The test test_train_model_uses_custom_gliner_trainer is too
rigid by checking the exact source substring "trainer =
Trainer(**trainer_kwargs)"; update it to robustly detect a Trainer instantiation
instead—either use a regex like r"trainer\s*=\s*Trainer\s*\(" on
MODEL_SOURCE.read_text(...) to allow spacing/arg changes, or parse the source
with ast and assert there is an Assign to a target named "trainer" whose value
is a Call to a Name "Trainer"; change the assertion to use that regex or AST
check so variable/spacing/inline-args variations won't break the test.
There was a problem hiding this comment.
Avoid silent truncation when coercing integer fields.
int(1.9) becomes 1, which can hide invalid configs. Prefer rejecting non‑integral floats/strings unless they represent whole numbers.
🔧 Suggested fix
if expected_type is int:
if isinstance(value, bool):
raise TypeError(f"Cannot coerce bool {value!r} to int")
- return int(value)
+ if isinstance(value, float):
+ if not value.is_integer():
+ raise TypeError(f"Cannot coerce non-integer float {value!r} to int")
+ return int(value)
+ if isinstance(value, str):
+ s = value.strip()
+ if not s.lstrip("+-").isdigit():
+ raise TypeError(f"Cannot coerce {value!r} to int")
+ return int(s)
+ return int(value)🧰 Tools
🪛 Ruff (0.15.1)
[warning] 196-196: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 199-199: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 203-203: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 210-210: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ptbr/config_cli.py` around lines 184 - 211, In _coerce_type, the int coercion
silently truncates non‑integral values (e.g. int(1.9) -> 1); update the
expected_type is int branch to reject non‑integral floats/strings: for float
inputs, allow only values where value.is_integer() is True (otherwise raise
TypeError); for string inputs, try parsing as int first, and if that fails try
parsing as float and accept only if that float is integral (value.is_integer()),
otherwise raise TypeError; keep the existing bool rejection and return
int(value) only when the value is demonstrably an integer.
There was a problem hiding this comment.
Documentation inconsistency: hidden_size default comment doesn't match value.
The comment states "Default: 512" but the actual value is 768. Either update the comment to reflect the intended default or adjust the value.
# Intermediate projection dimension after the encoder.
- # Default: 512
+ # Default: 768
hidden_size: 768🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ptbr/template.yaml` around lines 147 - 152, The comment for the YAML key
hidden_size is inconsistent with its value—update either the comment or the
value so they match: locate the hidden_size entry in template.yaml and either
change the inline comment "Default: 512" to the actual default "Default: 768" if
768 is intended, or revert the value from 768 to 512 if the intended default is
512; ensure the dropout comment/value pair remains unchanged and commit the
matching comment and value together.
There was a problem hiding this comment.
Documentation inconsistency: max_grad_norm default comment doesn't match value.
The comment states "Default: 1.0" but the actual value is 10.0. This is a significant difference that could affect gradient clipping behavior.
# Maximum gradient norm for clipping.
- # Default: 1.0
+ # Default: 10.0
max_grad_norm: 10.0📝 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.
| # Maximum gradient norm for clipping. | |
| # Default: 10.0 | |
| max_grad_norm: 10.0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ptbr/template.yaml` around lines 309 - 311, The documentation comment for the
max_grad_norm setting is inconsistent with its value; update the comment to
match the actual value or change the value to the intended default—specifically,
either change the comment to "Default: 10.0" to match max_grad_norm: 10.0, or
set max_grad_norm to 1.0 if the intended default is 1.0 so the code and comment
agree.
There was a problem hiding this comment.
Drop unused result assignments (F841)
Ruff flags these unused variables. You can call the function directly or assign to _.
🧹 Suggested fix
- result = _validate_section({}, _GLINER_RULES, "gliner_config", report)
+ _ = _validate_section({}, _GLINER_RULES, "gliner_config", report)
@@
- result = _validate_section(data, _GLINER_RULES, "gliner_config", report)
+ _ = _validate_section(data, _GLINER_RULES, "gliner_config", report)
@@
- result = _validate_section(data, _GLINER_RULES, "gliner_config", report)
+ _ = _validate_section(data, _GLINER_RULES, "gliner_config", report)📝 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.
| _ = _validate_section({}, _GLINER_RULES, "gliner_config", report) | |
| # model_name is REQUIRED | |
| errors = [i for i in report.errors if "model_name" in i.field] | |
| assert len(errors) == 1 | |
| assert "REQUIRED" in errors[0].message | |
| def test_default_fields_produce_warnings(self): | |
| report = ValidationReport() | |
| data = {"model_name": "some-model"} | |
| _ = _validate_section(data, _GLINER_RULES, "gliner_config", report) | |
| # Many fields should get defaults → warnings | |
| warning_fields = {i.field.split(".")[-1] for i in report.warnings} | |
| assert "name" in warning_fields | |
| assert "hidden_size" in warning_fields | |
| assert "dropout" in warning_fields | |
| assert report.is_valid # no errors | |
| def test_fully_specified_no_warnings(self): | |
| report = ValidationReport() | |
| data = _full_gliner_config() | |
| _ = _validate_section(data, _GLINER_RULES, "gliner_config", report) |
🧰 Tools
🪛 Ruff (0.15.1)
[error] 165-165: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
[error] 174-174: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
[error] 185-185: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ptbr/tests/test_config_cli.py` around lines 165 - 185, Tests assign the
return of _validate_section to the unused variable result in
test_default_fields_produce_warnings and test_fully_specified_no_warnings,
causing Ruff F841; remove the unused assignment or replace result with _ (e.g.,
call _validate_section(data, _GLINER_RULES, "gliner_config", report) without
assigning) so the function is still invoked but no unused variable remains;
update both occurrences referencing the variable name result in those tests.
There was a problem hiding this comment.
Fix Ruff ARG002 unused argument in _DummyModel.to.
Ruff flags the dtype argument as unused (Line 21). If linting is enforced in CI, this will fail.
🔧 Proposed fix
- def to(self, dtype=None):
+ def to(self, _dtype=None):
return self📝 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.
| def to(self, _dtype=None): | |
| return self |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 21-21: Unused method argument: dtype
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ptbr/tests/test_train_py.py` around lines 21 - 22, Ruff flags the unused
parameter in the _DummyModel.to method; rename the parameter to _dtype (or
prefix with an underscore) to mark it intentionally unused. Update the function
signature def to(self, _dtype=None): return self so the linter no longer reports
ARG002 while preserving behavior; reference the to method and the dtype
parameter when making the change.
There was a problem hiding this comment.
Avoid unused unpacked variable to satisfy Ruff RUF059.
Ruff flags spans as unused here; replace it with _ (or _spans) to keep lint clean.
🔧 Suggested fix
- spans, relations = decoder.decode(
+ _, relations = decoder.decode(🧰 Tools
🪛 Ruff (0.15.1)
[warning] 537-537: Unpacked variable spans is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_decoder.py` around lines 533 - 542, The test
`test_no_relations_when_not_requested` unpacks the result of
`SpanRelexDecoder.decode` into `spans, relations` but never uses `spans`,
triggering Ruff RUF059; change the unused variable to `_` (or `_spans`) in that
test so the unpack becomes `_, relations = decoder.decode(...)` to satisfy the
linter while leaving `SpanRelexDecoder.decode` behavior unchanged.
There was a problem hiding this comment.
Freeze class-level field sets (RUF012)
Ruff flags mutable class attributes. Use frozenset for these constants.
♻️ Suggested fix
- REQUIRED_GLINER_FIELDS = {
+ REQUIRED_GLINER_FIELDS = frozenset({
"focal_loss_alpha",
"focal_loss_gamma",
"focal_loss_prob_margin",
"label_smoothing",
"loss_reduction",
"negatives",
"masking",
"others_lr",
"others_weight_decay",
- }
+ })
@@
- CUSTOM_FIELDS = {
+ CUSTOM_FIELDS = frozenset({
"others_lr",
"others_weight_decay",
"focal_loss_alpha",
"focal_loss_gamma",
"focal_loss_prob_margin",
"label_smoothing",
"loss_reduction",
"negatives",
"masking",
- }
+ })Also applies to: 580-590
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 165-175: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_training_validation.py` around lines 165 - 175, The class-level
constant set REQUIRED_GLINER_FIELDS is mutable and should be frozen to satisfy
RUF012; replace its definition with a frozenset (e.g., frozenset({...})) to make
it immutable and do the same for the other mutable class-level sets referenced
around lines ~580-590 (identify their names in the file and convert them to
frozenset as well), ensuring you update any tests or usages that rely on
set-specific mutating methods if present.
| KNOWN_NON_FORWARDED = { | ||
| # These are used to build the model, not passed to train_model | ||
| "model", "data", | ||
| # These are read separately | ||
| "training.prev_path", "training.freeze_components", | ||
| } |
There was a problem hiding this comment.
Make KNOWN_NON_FORWARDED immutable (RUF012)
Ruff flags mutable class attributes. Use frozenset (or tuple) for constants.
♻️ Suggested fix
- KNOWN_NON_FORWARDED = {
+ KNOWN_NON_FORWARDED = frozenset({
# These are used to build the model, not passed to train_model
"model", "data",
# These are read separately
"training.prev_path", "training.freeze_components",
- }
+ })📝 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.
| KNOWN_NON_FORWARDED = frozenset({ | |
| # These are used to build the model, not passed to train_model | |
| "model", "data", | |
| # These are read separately | |
| "training.prev_path", "training.freeze_components", | |
| }) |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 474-479: Mutable default value for class attribute
(RUF012)
🤖 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 474 - 479, Replace the
mutable set KNOWN_NON_FORWARDED with an immutable collection: change the current
set literal assigned to KNOWN_NON_FORWARDED to a frozenset (or tuple) to satisfy
the linter; update the declaration where KNOWN_NON_FORWARDED is defined so that
any references to KNOWN_NON_FORWARDED elsewhere (e.g., tests expecting
membership checks) continue to work unchanged.
| def test_default_remove_unused_columns_is_true(self): | ||
| """The HF default for remove_unused_columns is True.""" | ||
| from gliner.training.trainer import TrainingArguments | ||
|
|
||
| args = TrainingArguments(output_dir="/tmp/_test_ruc") | ||
| assert args.remove_unused_columns is True, ( |
There was a problem hiding this comment.
Avoid hardcoded /tmp path in tests (S108)
Ruff flags the hardcoded /tmp/_test_ruc. Use tmp_path to keep tests portable and lint‑clean.
🧪 Suggested fix
- def test_default_remove_unused_columns_is_true(self):
+ def test_default_remove_unused_columns_is_true(self, tmp_path):
"""The HF default for remove_unused_columns is True."""
from gliner.training.trainer import TrainingArguments
- args = TrainingArguments(output_dir="/tmp/_test_ruc")
+ args = TrainingArguments(output_dir=str(tmp_path / "_test_ruc"))
assert args.remove_unused_columns is True, (
"HF TrainingArguments defaults remove_unused_columns to True"
)🧰 Tools
🪛 Ruff (0.15.1)
[error] 548-548: 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 - 549, The test
test_default_remove_unused_columns_is_true uses a hardcoded path
"/tmp/_test_ruc"; update the test to accept the pytest tmp_path fixture and
instantiate TrainingArguments with a temporary directory (e.g., str(tmp_path /
"_test_ruc")) instead of the literal "/tmp/_test_ruc" so the test is portable
and lint-clean; locate the test function
test_default_remove_unused_columns_is_true and replace the
TrainingArguments(output_dir=...) argument accordingly.
| CONFIG_FILES = [ | ||
| "config.yaml", | ||
| "config_span.yaml", | ||
| "config_token.yaml", | ||
| "config_decoder.yaml", | ||
| "config_biencoder.yaml", | ||
| "config_relex.yaml", | ||
| ] |
There was a problem hiding this comment.
Make CONFIG_FILES immutable (RUF012)
Ruff flags mutable class attributes. A tuple is sufficient here.
♻️ Suggested fix
- CONFIG_FILES = [
+ CONFIG_FILES = (
"config.yaml",
"config_span.yaml",
"config_token.yaml",
"config_decoder.yaml",
"config_biencoder.yaml",
"config_relex.yaml",
- ]
+ )📝 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.
| CONFIG_FILES = ( | |
| "config.yaml", | |
| "config_span.yaml", | |
| "config_token.yaml", | |
| "config_decoder.yaml", | |
| "config_biencoder.yaml", | |
| "config_relex.yaml", | |
| ) |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 813-820: Mutable default value for class attribute
(RUF012)
🤖 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 813 - 820, CONFIG_FILES is
defined as a mutable list; change it to an immutable tuple to satisfy Ruff
RUF012. Replace the list literal assigned to CONFIG_FILES with a tuple literal
(e.g., ("config.yaml", "config_span.yaml", ...)) so the constant is immutable
while keeping the same values; ensure any code that iterates over CONFIG_FILES
continues to work (tuples support iteration).
Provides load/validate/embed workflow for GLiNER datasets.
CLI: python -m ptbr --file-or-repo --validate --generate-label-embeddings
Module: from ptbr import prepare; result = prepare("data.json")
Compatible with all model variants (span, token, bi-encoder, decoder, relex, multitask).
https://claude.ai/code/session_01LVh5Xf67sRwVkTPNbpxAdH