102 separate linting workflow in ci - #103
Open
cpaniaguam wants to merge 43 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe PR adds a dedicated Ruff workflow, updates test workflow tooling, standardizes imports and file I/O, adjusts JAX and Torch trainer logic, and improves temporary test isolation. ChangesLANfactory maintenance updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lanfactory/trainers/jax_mlp.py (1)
272-279: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssign custom
lr_dictvalues.When
train_configcontains"lr_dict", this branch skips the only assignment toself.lr_dict.create_train_state()then raisesAttributeErroratself.lr_dict["init_value"]. Add anelsebranch that assignstrain_config["lr_dict"], and cover this configuration with a test.Suggested fix
if "lr_dict" not in train_config: self.lr_dict: dict[str, float] = { "init_value": 0.0002, "peak_value": 0.02, "end_value": 0.0, "exponent": 1.0, } + else: + self.lr_dict = train_config["lr_dict"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lanfactory/trainers/jax_mlp.py` around lines 272 - 279, Update the learning-rate configuration logic near the existing self.lr_dict initialization: add an else branch that assigns train_config["lr_dict"] to self.lr_dict when a custom configuration is provided, preserving the current defaults otherwise. Add a test covering custom lr_dict input and verifying create_train_state() uses it without raising AttributeError.
🧹 Nitpick comments (3)
.github/workflows/run_tests.yml (1)
26-34: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDo not delete the uv cache after setup-uv restores it.
setup-uv@v7defaults to cache restoration and saving. uv uses~/.cache/uvon Ubuntu by default. This step deletes the restored cache beforeuv sync, so each run loses the cache benefit and may redownload dependencies. Remove the uv cache deletion, or setenable-cache: falseexplicitly when a clean cache is required. (raw.githubusercontent.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/run_tests.yml around lines 26 - 34, Remove the ~/.cache/uv deletion from the “Clear all caches” workflow step so the cache restored by setup-uv remains available to uv sync; leave the other cache cleanup commands unchanged.pyproject.toml (1)
123-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the global BLE001 suppression.
BLE001 covers catch-all handlers like
except Exception; removingpyproject.toml's global ignore will surface these in each module that needs them. Add local# noqa: BLE001comments only where the broad handler is intentional, or narrow the handler if possible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 123 - 129, Remove "BLE001" from the global ignore list in the [tool.ruff.lint] configuration. Then address each newly reported broad exception handler by narrowing the caught exception where feasible, or adding a local # noqa: BLE001 only at intentionally broad handlers; do not retain the global suppression.Source: Coding guidelines
.github/workflows/linting_formatting.yml (1)
20-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin Ruff in CI or use the project tool path.
uvx ruffruns in a transient environment and does not usepyproject.toml’sruff>=0.14.4constraint. Useuv run ruffafter installing the dev group, or pin the CLI explicitly withuvx ruff@<version>so lint results cannot change on a fresh runner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/linting_formatting.yml around lines 20 - 24, Update the “Check styling” and “Check linting” workflow steps to invoke Ruff through the project-managed tool path with `uv run ruff`, ensuring the dev dependencies are installed beforehand; alternatively, pin both `uvx ruff` invocations to the project’s required Ruff version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/run_tests.yml:
- Around line 22-25: Update the run_tests job’s astral-sh/setup-uv@v7
configuration to pass the matrix value through its python-version input, using
matrix.python-version for uv sync and uv run pytest. Add an explicit Python 3.12
version to the test_notebooks job if it does not currently define one.
In `@src/lanfactory/trainers/jax_mlp.py`:
- Around line 123-127: Update the activation-function initialization and the
forward loop around self.activation_funs so the list retains one entry per
layer, storing None for "linear" activations. In the loop, invoke
self.activation_funs[i] only when that entry is non-None while preserving the
existing final-layer/linear skip behavior.
- Around line 126-127: Update the evaluation branch in the trainer method
containing the self.train and train_output_type check to use JAX’s numerically
stable log-sigmoid transform instead of manually computing -log(1 + exp(-x)).
Preserve the existing logits condition and assignment behavior while ensuring
finite log probabilities for large negative logits.
In `@src/lanfactory/trainers/torch_mlp.py`:
- Around line 91-92: The pickle deserialization paths in the trainer, including
the loading flow around self.file_ids[file_index] and the additional sites near
lines 106, 338, and 501, must not deserialize user-controlled files directly.
Replace pickle-based data/config loading with a safe format, or validate trusted
producer signatures and permitted paths before every pickle.load or pickle.loads
call, including inputs derived from training_data_folder and model/network/train
configuration paths.
- Around line 452-454: Update the logits branch in the relevant trainer method
around the final-layer expression to use a numerically stable log-sigmoid
operation, such as F.logsigmoid(logits) or -F.softplus(-logits), instead of
manually computing -torch.log(1 + torch.exp(-logits)). Preserve the existing
behavior and output for train_output_type == "logits".
In `@tests/test_jax_mlp.py`:
- Around line 46-48: Update the docstring of
test_mlp_jax_factory_raises_type_error to state that JaxMLPFactory raises
TypeError for an invalid network_config type, matching the pytest.raises
assertion.
---
Outside diff comments:
In `@src/lanfactory/trainers/jax_mlp.py`:
- Around line 272-279: Update the learning-rate configuration logic near the
existing self.lr_dict initialization: add an else branch that assigns
train_config["lr_dict"] to self.lr_dict when a custom configuration is provided,
preserving the current defaults otherwise. Add a test covering custom lr_dict
input and verifying create_train_state() uses it without raising AttributeError.
---
Nitpick comments:
In @.github/workflows/linting_formatting.yml:
- Around line 20-24: Update the “Check styling” and “Check linting” workflow
steps to invoke Ruff through the project-managed tool path with `uv run ruff`,
ensuring the dev dependencies are installed beforehand; alternatively, pin both
`uvx ruff` invocations to the project’s required Ruff version.
In @.github/workflows/run_tests.yml:
- Around line 26-34: Remove the ~/.cache/uv deletion from the “Clear all caches”
workflow step so the cache restored by setup-uv remains available to uv sync;
leave the other cache cleanup commands unchanged.
In `@pyproject.toml`:
- Around line 123-129: Remove "BLE001" from the global ignore list in the
[tool.ruff.lint] configuration. Then address each newly reported broad exception
handler by narrowing the caught exception where feasible, or adding a local #
noqa: BLE001 only at intentionally broad handlers; do not retain the global
suppression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb1bc6fe-8321-42a1-aee1-a00e202bb286
📒 Files selected for processing (30)
.github/workflows/linting_formatting.yml.github/workflows/run_tests.yml.gitignorepyproject.tomlsrc/lanfactory/__init__.pysrc/lanfactory/cli/download_hf.pysrc/lanfactory/cli/jax_train.pysrc/lanfactory/cli/torch_train.pysrc/lanfactory/cli/upload_hf.pysrc/lanfactory/cli/utils.pysrc/lanfactory/config/__init__.pysrc/lanfactory/hf/__init__.pysrc/lanfactory/hf/download.pysrc/lanfactory/hf/upload.pysrc/lanfactory/network_inspectors/__init__.pysrc/lanfactory/network_inspectors/api.pysrc/lanfactory/network_inspectors/config.pysrc/lanfactory/network_inspectors/loaders.pysrc/lanfactory/onnx/__init__.pysrc/lanfactory/trainers/__init__.pysrc/lanfactory/trainers/jax_mlp.pysrc/lanfactory/trainers/torch_mlp.pysrc/lanfactory/utils/__init__.pysrc/lanfactory/utils/mlflow_utils.pysrc/lanfactory/utils/util_funs.pytests/conftest.pytests/constants.pytests/test_cli_utils.pytests/test_end_to_end_jax.pytests/test_jax_mlp.py
💤 Files with no reviewable changes (4)
- tests/constants.py
- src/lanfactory/network_inspectors/config.py
- src/lanfactory/cli/download_hf.py
- src/lanfactory/cli/upload_hf.py
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 6 to 7. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v6...v7) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tests/test_jax_mlp.py:47
- The docstring still says the factory raises ValueError, but the test now asserts a TypeError. Update the docstring to match the actual exception type to avoid misleading documentation.
"""Test JaxMLPFactory raises ValueError for invalid network_config type."""
.github/workflows/run_tests.yml:64
- The
test_notebooksjob no longer pins a Python version. This makes the job sensitive to GitHub runner defaults/changes and can break if the default Python is outside the supported range. Configure the Python version viasetup-uv(as done in the matrix test job).
- name: Install uv
uses: astral-sh/setup-uv@v7
with:
version: "0.12.0"
.github/workflows/linting_formatting.yml:18
- This workflow also relies on the runner’s default Python. Pinning the Python version via
setup-uvimproves reproducibility and avoids CI breakage when runner defaults change.
- name: Install uv
uses: astral-sh/setup-uv@v7
with:
version: "0.12.0"
…thub_actions/actions/setup-python-7
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/lanfactory/hf/init.py:11
- Module-level imports come after assignments (
DEFAULT_REPO_ID,VALID_NETWORK_TYPES), which will trigger Ruff E402 (imports not at top of file) in a default ruff configuration. Reorder so imports come immediately after the module docstring.
DEFAULT_REPO_ID = "franklab/HSSM"
VALID_NETWORK_TYPES = ("lan", "cpn", "opn")
from lanfactory.hf.download import download_model
from lanfactory.hf.model_card import (
tests/test_jax_mlp.py:47
- The test name and assertion expect a
TypeError, but the docstring still saysValueError, which is misleading when reading failures.
"""Test JaxMLPFactory raises ValueError for invalid network_config type."""
src/lanfactory/trainers/jax_mlp.py:451
- If MLflow logging fails once, this will print an error every 100 steps for the rest of training, which can spam CI logs and slow runs. Consider disabling MLflow logging after the first failure (or gating the message behind
verbose).
except Exception as e:
print(f"Failed to log metric to MLflow: {e}")
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 49 changed files in this pull request and generated no new comments.
Suppressed comments (3)
.github/workflows/linting_formatting.yml:19
- The lint job doesn’t pin a Python version. Since the project requires Python >=3.12 (pyproject.toml:19), this job can become non-deterministic (or fail) depending on the runner’s default Python. Pin a supported Python version the same way the test workflow does.
- name: Install uv
uses: astral-sh/setup-uv@v7
with:
version: "0.12.0"
tests/test_mlflow_integration.py:55
- In teardown, the exception handler calls
mlflow.set_tracking_uri(...)again without guarding it. If resetting the tracking URI ever fails, this can cause teardown to raise and mask the actual test failure; prefer a best-effort reset that cannot raise from teardown.
src/lanfactory/cli/utils.py:104 yaml.safe_load()is being given raw bytes fromPath(...).read_bytes(). PyYAML’s primary interface is text / file-like streams; decoding explicitly avoids cross-version quirks and makes the expected encoding clear.
def _get_train_network_config(yaml_config_path: str | Path | None = None, net_index=0):
if yaml_config_path is not None:
basic_config = yaml.safe_load(Path(yaml_config_path).read_bytes())
network_type = basic_config["NETWORK_TYPE"]
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 49 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/lanfactory/trainers/jax_mlp.py:128
-jnp.log(1 + jnp.exp(-x))can overflow for large-magnitude logits, which can introduceinf/nanduring inference. JAX exposes a numerically stablelog_sigmoidfor this transformation.
if i != (len(self.layers) - 1) or self.activations[i] != "linear":
x = self.activation_funs[i](x)
if (not self.train) and (self.train_output_type == "logits"):
x = -jnp.log(1 + jnp.exp(-x))
.github/workflows/linting_formatting.yml:19
- This workflow doesn't set a Python version. Since the project targets Python >=3.12, relying on
ubuntu-latest's default Python risks CI breakage when GitHub updates the runner image. Consider explicitly settingpython-versioninsetup-uv.
- name: Install uv
uses: astral-sh/setup-uv@v7
with:
version: "0.12.0"
src/lanfactory/trainers/torch_mlp.py:455
-torch.log(1 + torch.exp(-x))can overflow for large-magnitude logits (e.g., float32 whenxis very negative), producinginf/nan. PyTorch provides a numerically stable implementation forlog(sigmoid(x)).
if self.training or self.train_output_type == "logprob":
return self.layers[-1](x)
elif self.train_output_type == "logits":
return -torch.log(
1 + torch.exp(-self.layers[-1](x))
) # log ( 1 / (1 + exp(-x))), where x = log(p / (1 - p))
else:
.github/workflows/run_tests.yml:64
test_notebooksno longer setspython-versionafter removingactions/setup-python. Ifubuntu-latest's default Python drifts below the supported range, this job can start failing unexpectedly. Pinning a Python version here would make the job deterministic.
- name: Install uv
uses: astral-sh/setup-uv@v7
with:
version: "0.12.0"
…ons/setup-python-7 chore(deps): bump actions/setup-python from 6 to 7
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/run_tests.yml:
- Line 64: Update the Python setup step for the test_notebooks job to use the
explicitly defined Python version "3.12" instead of the undefined
matrix.python-version reference; do not add a matrix unless this job is intended
to run across multiple versions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60970c9d-ac41-4bae-9034-249a9a6484ee
📒 Files selected for processing (2)
.github/workflows/run_tests.ymltests/test_bayesflow_nle_export.py
💤 Files with no reviewable changes (1)
- tests/test_bayesflow_nle_export.py
| with: | ||
| version: "0.6.5" | ||
| version: "0.12.0" | ||
| python-version: ${{ matrix.python-version }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use a defined Python version for test_notebooks.
test_notebooks does not define a strategy.matrix, so ${{ matrix.python-version }} is undefined. This causes the workflow validation error reported by actionlint and prevents the notebook job from selecting its intended interpreter. Set this to "3.12" or add a matrix to this job.
Proposed fix
- python-version: ${{ matrix.python-version }}
+ python-version: "3.12"🧰 Tools
🪛 actionlint (1.7.12)
[error] 64-64: property "python-version" is not defined in object type {}
(expression)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/run_tests.yml at line 64, Update the Python setup step for
the test_notebooks job to use the explicitly defined Python version "3.12"
instead of the undefined matrix.python-version reference; do not add a matrix
unless this job is intended to run across multiple versions.
Source: Linters/SAST tools
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 49 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/lanfactory/trainers/torch_mlp.py:454
- Computing log-sigmoid via
-torch.log(1 + torch.exp(-x))is numerically unstable for large-magnitude inputs and can overflow/underflow to inf/nan. PyTorch provides a stable implementation.
elif self.train_output_type == "logits":
return -torch.log(
1 + torch.exp(-self.layers[-1](x))
) # log ( 1 / (1 + exp(-x))), where x = log(p / (1 - p))
.github/workflows/linting_formatting.yml:18
- The linting workflow doesn't pin a Python version. Since the project targets Python >=3.12, relying on whatever happens to be preinstalled on
ubuntu-latestcan cause CI to start failing when the runner image changes.
uses: astral-sh/setup-uv@v7
with:
version: "0.12.0"
tests/test_mlflow_integration.py:55
- If resetting MLflow's tracking URI fails during cleanup, the fallback
mlflow.set_tracking_uri("file:./mlruns")can also raise and fail the test teardown. Teardown should not introduce new failures; swallow errors if both resets fail.
src/lanfactory/trainers/jax_mlp.py:128 - Computing log-sigmoid via
-jnp.log(1 + jnp.exp(-x))is numerically unstable for large-magnitude inputs and can overflow/underflow. Use JAX's stablelog_sigmoid.
if (not self.train) and (self.train_output_type == "logits"):
x = -jnp.log(1 + jnp.exp(-x))
| with: | ||
| version: "0.6.5" | ||
| version: "0.12.0" | ||
| python-version: ${{ matrix.python-version }} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Improvements
Quality