maintenance: relax & organize dependencies, simplify function signatures#19
maintenance: relax & organize dependencies, simplify function signatures#19anmorgunov merged 16 commits intomainfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Python 3.13 | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: '3.13' | ||
|
|
||
| - name: Install uv | ||
| run: | | ||
| curl -LsSf https://astral.sh/uv/install.sh | sh | ||
| echo "$HOME/.cargo/bin" >> $GITHUB_PATH | ||
|
|
||
| - name: Cache dependencies | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: | | ||
| ~/.cache/uv | ||
| ~/.uv | ||
| .venv | ||
| key: ${{ runner.os }}-uv-${{ hashFiles('pyproject.toml') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-uv- | ||
|
|
||
| - name: Create and activate virtual environment | ||
| run: | | ||
| uv venv | ||
| echo "$PWD/.venv/bin" >> $GITHUB_PATH | ||
|
|
||
| - name: Install dependencies | ||
| run: uv pip install -e ".[dev]" | ||
|
|
||
| - name: Run tests | ||
| run: pytest -v No newline at end of file |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To fix the issue, we will add a permissions block at the root of the workflow file. This block will specify the minimal permissions required for the workflow to function. Based on the provided steps, the workflow primarily interacts with the repository to read contents (e.g., pyproject.toml for caching). Therefore, we will set contents: read as the permission. If additional permissions are required in the future, they can be added explicitly.
| @@ -3,2 +3,4 @@ | ||
| on: [pull_request] | ||
| permissions: | ||
| contents: read | ||
| jobs: |
|
@coderabbitai let's do a review |
|
Tip For best results, initiate chat on the files or code changes. @anmorgunov I'll review the changes in this maintenance PR, focusing on the dependency updates and function signature simplifications. [review] |
WalkthroughThis update refactors code quality and CI workflows, revises dependency management, and improves type and file handling across the codebase. Major changes include splitting linting and testing workflows, removing pandas in favor of native structures, enforcing stricter iterable checks, updating dependency specifications, and enhancing model and beam search interfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Actions
participant Linter (ruff/mypy)
participant Tester (pytest)
User->>GitHub Actions: Push or PR event
GitHub Actions->>Linter (ruff/mypy): Run linting workflow
Linter (ruff/mypy)-->>GitHub Actions: Report lint/type status
GitHub Actions->>Tester (pytest): Run testing workflow (on PR)
Tester (pytest)-->>GitHub Actions: Report test results
sequenceDiagram
participant User
participant ModelLoader
participant BeamSearchFactory
participant RoutesProcessing
User->>ModelLoader: load_published_model(name, ckpt_dir, device)
ModelLoader-->>User: model
User->>RoutesProcessing: instantiate(config)
User->>BeamSearchFactory: create_beam_search(model, beam_size, rds)
BeamSearchFactory-->>User: BeamSearch object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
♻️ Duplicate comments (1)
.github/workflows/testing.yml (1)
3-6: Add explicit permissions to limit GITHUB_TOKEN scopeThe workflow should explicitly define permissions for security best practices.
on: [pull_request] + +permissions: + contents: read + jobs:
🧹 Nitpick comments (9)
use-examples/eval-subset.py (1)
10-10: Consider simplifying redundant conditional logic.Since
__mode__is constrained to only "local" or "cluster" by the assertion on line 8, the condition__mode__ == "local" or __mode__ in ["cluster"]will always be true and could be removed entirely.-if __mode__ == "local" or __mode__ in ["cluster"]: - base_path = Path(__file__).resolve().parent.parent +base_path = Path(__file__).resolve().parent.parent.github/workflows/linting.yml (1)
49-49: Add missing newline at end of file.The file should end with a newline character following Unix text file conventions.
- run: mypy . + run: mypy . +src/directmultistep/generation/eval.py (1)
179-184: Shadowingpathlib.Pathwith loop variablepathmay confuse readersInside the beam-search loop, the variable name
pathshadows the importedPathclass frompathlib, which is frequently referenced elsewhere in the codebase. Renaming this loop variable (e.g. topath_start_BL) will improve readability and avoid accidental misuse..pre-commit-config.yaml (1)
23-25: Minor YAML hygiene – add newline at EOF & quote regexYAML-lint flags a missing trailing newline. While touching the file, quoting the regex avoids future parsing surprises.
types: [python] -exclude: ^(tests/) -args: ["--config-file=pyproject.toml"] +# path regexp is interpreted by pre-commit, keep it explicit +exclude: "^(tests/)" +args: ["--config-file=pyproject.toml"] +src/directmultistep/generation/generation.py (1)
194-195: Walrus operator here hurts clarity more than it helpsThe one-off assignment within
append()saves one line but obscures intent. A simple two-step approach is easier to read and debug.- beam_idxs_BS1_nt[B_idx][S_idx].append(chosen_idx := top_k_BS_nt[B_idx][S_idx]) - beam_log_probs_BS_nt[B_idx][S_idx] += np.log(normalized_probs_BLS[B_idx, -1, chosen_idx].item()) + chosen_idx = top_k_BS_nt[B_idx][S_idx] + beam_idxs_BS1_nt[B_idx][S_idx].append(chosen_idx) + beam_log_probs_BS_nt[B_idx][S_idx] += np.log( + normalized_probs_BLS[B_idx, -1, chosen_idx].item() + ).github/workflows/testing.yml (1)
42-42: Fix formatting issuesRemove trailing spaces and add a newline at the end of the file.
- run: pytest -v + run: pytest -vsrc/directmultistep/analysis/training.py (3)
12-26: Fix docstring typo and improve error handling documentationThe docstring has a typo and could be more descriptive about the silent failure behavior.
-def _cast_numeric_values(data: list[dict[str, Any]]) -> list[dict[str, Any]]: - """attempts to cognize string values into floats. fails silently.""" +def _cast_numeric_values(data: list[dict[str, Any]]) -> list[dict[str, Any]]: + """Convert string values to floats where possible, setting None on failure."""
91-98: Improve function docstringThe docstring should be more professional.
- """makes a graph. you get it.""" + """Plot training curves for multiple runs with configurable axes scaling."""
135-136: Improve function docstringThe docstring should be more descriptive.
- """makes another graph. also obvious.""" + """Plot learning rate curves for multiple training runs."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.github/workflows/linting.yml(2 hunks).github/workflows/testing.yml(1 hunks).pre-commit-config.yaml(1 hunks)data/process.py(5 hunks)pyproject.toml(3 hunks)scripts/solve_compounds.py(1 hunks)src/directmultistep/analysis/paper/linear_vs_convergent.py(5 hunks)src/directmultistep/analysis/training.py(4 hunks)src/directmultistep/generate.py(5 hunks)src/directmultistep/generation/eval.py(2 hunks)src/directmultistep/generation/generation.py(1 hunks)src/directmultistep/generation/tensor_gen.py(4 hunks)src/directmultistep/model/components/moe.py(2 hunks)src/directmultistep/model/config.py(2 hunks)src/directmultistep/model/factory.py(2 hunks)src/directmultistep/training/lightning.py(1 hunks)src/directmultistep/utils/io.py(4 hunks)src/directmultistep/utils/post_process.py(4 hunks)use-examples/eval-subset.py(1 hunks)use-examples/paper-figures.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/directmultistep/model/config.py (1)
src/directmultistep/generation/eval.py (1)
load(102-115)
data/process.py (1)
src/directmultistep/utils/io.py (1)
save_dataset_sm(75-84)
src/directmultistep/model/factory.py (1)
src/directmultistep/model/config.py (4)
EncoderAConfig(90-99)MoEEncoderConfig(103-114)TransformerConfig(11-71)MoEDecoderConfig(75-86)
src/directmultistep/generation/tensor_gen.py (1)
src/directmultistep/generation/generation.py (1)
decode(148-235)
src/directmultistep/analysis/training.py (1)
src/directmultistep/analysis/style.py (1)
apply_development_style(161-170)
🪛 actionlint (1.7.7)
.github/workflows/testing.yml
13-13: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 GitHub Check: CodeQL
.github/workflows/testing.yml
[warning] 6-42: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 YAMLlint (1.37.1)
.github/workflows/testing.yml
[error] 42-42: no new line character at the end of file
(new-line-at-end-of-file)
[error] 42-42: trailing spaces
(trailing-spaces)
.github/workflows/linting.yml
[error] 49-49: no new line character at the end of file
(new-line-at-end-of-file)
.pre-commit-config.yaml
[error] 25-25: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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: lint
🔇 Additional comments (34)
src/directmultistep/training/lightning.py (2)
1-1: LGTM: Import modernizationGood update to use
collections.abc.Callableinstead oftyping.Callable, which is the recommended approach for Python 3.9+ and aligns with PEP 585.
190-197: LGTM: Simplified scheduler configurationThe removal of the
verbose=Falseparameter simplifies the LambdaLR instantiation. This is likely due to either the parameter being deprecated or becoming the default behavior in newer PyTorch versions.use-examples/paper-figures.py (1)
158-158: LGTM: Added strict length checkingGood addition of
strict=Trueto the zip function. This ensures thatconfigsanddatasetshave matching lengths and will raise aValueErrorif they don't, preventing potential silent bugs from mismatched iterables.src/directmultistep/utils/io.py (4)
96-98: LGTM: Added strict length checking to zip operationsExcellent addition of
strict=Trueto both zip calls. This ensures that the dictionary keys and values have matching lengths, preventing silent bugs from mismatched data structures.
110-110: LGTM: Simplified dictionary iterationGood simplification by removing the unnecessary
.keys()call. Iterating directly over the dictionary is equivalent and more concise.
126-126: LGTM: Removed redundant file mode specificationThe explicit
"r"mode is redundant since it's the default foropen(). This change simplifies the code while maintaining the same functionality.
175-175: LGTM: Removed redundant file mode specificationSame improvement as above - removing the explicit
"r"mode since it's the default behavior.scripts/solve_compounds.py (1)
45-45: LGTM: Consistent removal of redundant file mode specificationsAll four changes consistently remove the explicit
"r"mode fromopen()calls, aligning with the broader codebase cleanup. Since"r"is the default mode, these changes simplify the code while maintaining identical functionality.Also applies to: 48-48, 56-56, 60-60
src/directmultistep/model/config.py (2)
3-3: LGTM: Modernized typing importsGood update to remove the unused
Typeimport from the typing module, keeping only the necessaryLiteralandTypeVarimports. This aligns with the type annotation modernization in the codebase.
60-60: LGTM: Updated type annotation to use built-in typeExcellent modernization of the type annotation from
Type[T]totype[T], following PEP 585 recommendations for Python 3.9+. This change is consistent with the broader effort to modernize type annotations across the codebase.src/directmultistep/model/components/moe.py (2)
194-194: Good modernization of super() call.The simplified
super()syntax is the preferred approach in Python 3, improving readability while maintaining the same functionality.
225-225: Clean simplification of conditional logic.The ternary operator effectively condenses the capacity limiting logic while maintaining clarity and the same behavior.
src/directmultistep/utils/post_process.py (4)
1-1: Good modernization of type imports.Moving from
typing.Iteratortocollections.abc.Iteratorfollows PEP 585 recommendations and is the preferred approach for Python 3.9+.
106-108: Excellent addition of strict zip validation.Adding
strict=Trueensures thatpaths_NS2nandcorrect_pathshave matching lengths, preventing silent data truncation and potential bugs from misaligned data.
153-153: Minor formatting improvement.Adding proper spacing around the multiplication operator improves code readability and follows PEP 8 style guidelines.
475-475: Consistent application of strict zip validation.Adding
strict=Trueensuresmatch_accuracyandn_steps_listhave matching lengths, maintaining data integrity in the step-length correlation analysis.src/directmultistep/analysis/paper/linear_vs_convergent.py (5)
333-333: Consistent strict zip validation for plotting data.Adding
strict=Trueensures proper alignment betweencategoriesandpositionsin the plotting logic, preventing potential visualization errors.
341-341: Proper validation of results and trace names alignment.The strict zip ensures each result set has a corresponding trace name, preventing data misalignment in the comparative plots.
411-411: Important three-way data alignment validation.The strict zip on
result_paths,datasets, andconfigsensures all three data sources are properly aligned, which is critical for accurate analysis results.
472-472: Consistent three-way validation in subplot function.Maintains the same strict alignment validation as the main plotting function, ensuring consistency across different visualization approaches.
485-485: Good use of underscore prefix for unused variable.Prefixing
_route_typewith an underscore clearly indicates the variable is intentionally unused, following Python naming conventions..github/workflows/linting.yml (3)
1-1: Good workflow name clarification.Renaming to "Linting" better reflects the focused purpose of this workflow, especially with testing moved to a separate workflow file.
6-6: Consistent job naming.The
lintjob name aligns well with the workflow's focused purpose and improves clarity.
40-40: Good modernization of dependency installation.Using
uv syncis the preferred approach for dependency synchronization with uv, providing better handling of lock files and dependency resolution.src/directmultistep/generation/eval.py (1)
68-70: Prefer explicit exception overassertfor runtime-critical validation
assertstatements are stripped when Python is run with the-Oflag, which would silently bypass the dataset-name validation in production. Replace with an explicit check and raised exception to guarantee enforcement.-assert self.eval_dataset in allowed_ds, ( - f"Eval dataset {self.eval_dataset} not in allowed datasets: {allowed_ds}" -) +if self.eval_dataset not in allowed_ds: + raise ValueError( + f"Eval dataset {self.eval_dataset} is not among the allowed datasets {allowed_ds}" + )[ suggest_essential_refactor ]
data/process.py (1)
98-100: 👍 Context manager adopted for pickle dumpGood catch converting the raw
open()call into awithblock – aligns with the new lint rule and ensures file handles are closed.src/directmultistep/generation/tensor_gen.py (2)
54-61: Well-designed backward-compatible API enhancementThe addition of
progress_barandtoken_processorparameters maintains backward compatibility while adding useful functionality. The optional token processor pattern is a clean way to allow custom token processing.
96-97: Clean conditional progress bar implementationGood use of conditional expression to handle the progress bar display based on the parameter.
src/directmultistep/generate.py (2)
35-37: Good addition of device control parameterThe
force_deviceparameter provides flexibility for explicit device selection while maintaining backward compatibility with the default behavior.
59-73: Improved modularity in beam search creationExcellent refactoring to accept
RoutesProcessingobject directly instead of re-reading the config file. This reduces I/O operations and centralizes configuration handling.pyproject.toml (3)
5-12: Good dependency management improvementsSwitching from exact pins to minimum version requirements provides more flexibility while maintaining compatibility. The removal of pandas aligns with the codebase refactoring.
25-27: Proper build system configurationGood addition of build system requirements for package distribution.
75-83: Comprehensive linting configurationThe explicit rule selection provides clear visibility into which linting rules are enforced. Ignoring E501 is appropriate given the 120-character line length setting.
src/directmultistep/analysis/training.py (1)
28-50: Good refactoring to remove pandas dependencyThe implementation using csv.DictReader and native Python structures is clean and efficient. The type casting approach handles missing data appropriately.
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: '3.13' | ||
|
|
There was a problem hiding this comment.
Update actions/setup-python to v5
The action version is outdated and may not run properly on newer GitHub Actions runners.
- - name: Set up Python 3.13
- uses: actions/setup-python@v4
- with:
- python-version: '3.13'
+ - name: Set up Python 3.13
+ uses: actions/setup-python@v5
+ with:
+ python-version: '3.13'📝 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.
| uses: actions/setup-python@v4 | |
| with: | |
| python-version: '3.13' | |
| - name: Set up Python 3.13 | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: '3.13' |
🧰 Tools
🪛 actionlint (1.7.7)
13-13: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/testing.yml around lines 13 to 16, the setup-python action
is using version v4, which is outdated. Update the version from
actions/setup-python@v4 to actions/setup-python@v5 to ensure compatibility with
newer GitHub Actions runners and improve reliability.
| if not isinstance(self.config.encoder, EncoderAConfig | MoEEncoderConfig): | ||
| raise TypeError("Encoder config must be either EncoderAConfig or MoEEncoderConfig") | ||
| if not isinstance(self.config.decoder, (TransformerConfig, MoEDecoderConfig)): | ||
| if not isinstance(self.config.decoder, TransformerConfig | MoEDecoderConfig): | ||
| raise TypeError("Decoder config must be either TransformerConfig or MoEDecoderConfig") | ||
|
|
There was a problem hiding this comment.
isinstance(obj, A | B) is invalid – will raise TypeError at runtime
Using the PEP-604 union operator inside isinstance is not supported. Pass a tuple of classes instead.
- if not isinstance(self.config.encoder, EncoderAConfig | MoEEncoderConfig):
+ if not isinstance(self.config.encoder, (EncoderAConfig, MoEEncoderConfig)):
raise TypeError("Encoder config must be either EncoderAConfig or MoEEncoderConfig")
- if not isinstance(self.config.decoder, TransformerConfig | MoEDecoderConfig):
+ if not isinstance(self.config.decoder, (TransformerConfig, MoEDecoderConfig)):
raise TypeError("Decoder config must be either TransformerConfig or MoEDecoderConfig")📝 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 not isinstance(self.config.encoder, EncoderAConfig | MoEEncoderConfig): | |
| raise TypeError("Encoder config must be either EncoderAConfig or MoEEncoderConfig") | |
| if not isinstance(self.config.decoder, (TransformerConfig, MoEDecoderConfig)): | |
| if not isinstance(self.config.decoder, TransformerConfig | MoEDecoderConfig): | |
| raise TypeError("Decoder config must be either TransformerConfig or MoEDecoderConfig") | |
| if not isinstance(self.config.encoder, (EncoderAConfig, MoEEncoderConfig)): | |
| raise TypeError("Encoder config must be either EncoderAConfig or MoEEncoderConfig") | |
| if not isinstance(self.config.decoder, (TransformerConfig, MoEDecoderConfig)): | |
| raise TypeError("Decoder config must be either TransformerConfig or MoEDecoderConfig") |
🤖 Prompt for AI Agents
In src/directmultistep/model/factory.py around lines 91 to 95, the use of the
PEP-604 union operator (A | B) inside isinstance calls is invalid and causes a
TypeError at runtime. Replace the union operator with a tuple of classes in the
isinstance checks, for example, use isinstance(self.config.encoder,
(EncoderAConfig, MoEEncoderConfig)) and similarly for the decoder check.
1 participant
Summary by CodeRabbit
New Features
Refactor
Style
Chores