fix: resolve ruff linting issues and add CI workflow - #133
Conversation
|
Found 3 changed notebooks. Review the changes at https://app.gitnotebooks.com/sensorium-competition/experanto/pull/133 |
There was a problem hiding this comment.
Pull request overview
This PR primarily focuses on cleanup and lint-readiness across the Experanto codebase by removing unused imports/variables, simplifying a few error strings, and adding a new Ruff CI workflow.
Changes:
- Remove unused imports/variables across core modules (
utils,datasets,dataloaders,experiment,interpolators,configs). - Minor string cleanup in error/exception messages.
- Add a Ruff GitHub Actions workflow and ignore a local environment directory in
.gitignore.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
experanto/utils.py |
Removes unused imports and unused local vars in add_behavior_as_channels. |
experanto/intervals.py |
Simplifies an error return string. |
experanto/interpolators.py |
Removes an unused import and simplifies NotImplementedError message strings. |
experanto/experiment.py |
Removes unused imports/types. |
experanto/datasets.py |
Removes unused imports and a dead local variable; keeps dataset logic intact. |
experanto/dataloaders.py |
Removes unused imports and timing variable. |
experanto/configs.py |
Removes unused Hydra/open_dict imports; keeps YAML-based default config loading. |
examples/sensorium/sensorium_min_example.ipynb |
Removes unused imports in the example notebook. |
examples/sensorium/experiment.ipynb |
Removes unused imports in the example notebook. |
examples/sensorium/data.ipynb |
Removes unused imports in the example notebook. |
.gitignore |
Ignores gsoc_env/. |
.github/workflows/ruff.yml |
Adds a Ruff lint workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Hi @binary69 , thanks for the PR!
It looks pretty good, but I have very few things I am wondering about:
- I would have expected that ruff is added with a version constraint into pyproject.toml's dev-group
- I also would have expected that the default configuration of ruff is set in pyproject.toml's tool.ruff section, particularly setting line-length, target-version, exclude, src, lint.select, lint.ignore and possibly others if helpful/necessary. Please try to match the settings with isort and black, so they don't counteract on each other. In another project, we have
lint.select = ["E", "F", "B", "UP", "C4"]andlint.ignore = ["E501"] # ignore line length, but I am not fully sure thats correct here as well. With this the command in ruff.yml should also be changed toruff checkonly, so that it matches whatever users are running locally as well. It can be frustrating to see different output of ruff locally or in Github workflows. - ruff.yml looks good, but different than the other workflows making it hard to parse whats different. If possible, please try to match code style, naming, order of actions, etc. (either changing ruff.yml or the other workflows)
- I agree that examples folder should be excluded, but if possible, please also solve the linting issues of ruff there as well
Very happy to accept after changes to those things or feedback why it doesn't make sense
|
I've fixed all the linting issues in |
|
The ruff auto-fix converted Union annotation in |
pollytur
left a comment
There was a problem hiding this comment.
please resolve the strict in zip
strict=False is the default behaviour, so I would rather remove it
However in most of the cases (apart from the data loader helpers!) we assume that the lists are equal length and setting strict=True might help
I have made corresponding suggestions.
Lets address and happy to merge afterwards
Co-authored-by: Polina Turishcheva <49938348+pollytur@users.noreply.github.com>
Co-authored-by: Polina Turishcheva <49938348+pollytur@users.noreply.github.com>
Co-authored-by: Polina Turishcheva <49938348+pollytur@users.noreply.github.com>
Co-authored-by: Polina Turishcheva <49938348+pollytur@users.noreply.github.com>
Co-authored-by: Polina Turishcheva <49938348+pollytur@users.noreply.github.com>
Co-authored-by: Polina Turishcheva <49938348+pollytur@users.noreply.github.com>
Co-authored-by: Polina Turishcheva <49938348+pollytur@users.noreply.github.com>
Co-authored-by: Polina Turishcheva <49938348+pollytur@users.noreply.github.com>
|
made the changes as mentioned :) |
closes #129
Removed 53 unused imports and fixed style issues automatically using ruff --fix
Manually removed unused local variables (start_time, chunk_s, c_eye, c_tread)
Removed redefined DEFAULT_MODALITY_CONFIG in datasets.py
Added
.github/workflows/ruff.ymlto run ruff automatically on every PR and pushPinned ruff to v0.15.6 for deterministic CI results
Added trigger guard to avoid duplicate CI runs
All 360 tests passing after changes
The remaining 20 errors are in example notebook files (.ipynb).
These involve shell commands and cross-cell variable references.
that are valid in notebook context but flagged by ruff.
Left untouched to avoid breaking example notebooks.