102 separate linting workflow in ci - #103
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:
|
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>
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
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}")
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"]
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
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 }} |
|
One small request from the drift-detection side (#107), if it's easy to fold in here. The new Today it gets lint coverage for free, because linting lives inside on:
pull_request:
workflow_call:No other change needed — I'll add the Also, for whenever this lands: #107 edits the same region of |
Summary by CodeRabbit
Improvements
Quality