Add notebooks, tests, and environment setup with improved project metadata for refactor initial release#591
Conversation
… CLI/without a config
Add rule to exlude notebooks (.ipynb) files from pre commit hook
- Modify pyproject.toml file with more detailed metadata, information to install dependencies using requirements.txt - Modify pyproject.toml to extend isort config with know first-party so local module are seperated in the imports - Add the requirements.txt files with pinned version
- Add tests for SegmentationUnetPlus model, CSVDataModule and runs a Trainer.fit() with fast_dev_run - Add ruff exception to ignore asserts in the tests folder only
- Modify relative imports with absolute imports - Add TC002 exception to allow imports without TYPE_CHECKING
- Switching to pip only in github action - Fix requirements.txt file using == instead of =
LucaRom
force-pushed
the
notebook_n_base
branch
from
bd8866b to
f51a8ee
Compare
|
The force push was to squash the last commits that were all trial and error to get the github actions on push working and running the test :) |
LucaRom
requested review from
CharlesAuthier,
mpelchat04,
ms888ekb and
valhassan
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: create environment | ||
| uses: conda-incubator/setup-miniconda@v3 |
There was a problem hiding this comment.
Since we already use Miniconda as the environment for running our GDL scripts, should we also run the tests inside the same Miniconda environment? This would help ensure consistency between how the scripts are run and how they’re tested.
There was a problem hiding this comment.
Yeah, it was my first intention, but it kept failing for some reason. I will try and mess around on another branch to see if I can do fix it. Otherwise, I think it can be tackle later with a dedicated issue.
There was a problem hiding this comment.
Coming back on this as I forgot the real reasoning behing this when first answering.
The repo is built using a pyproject.toml file that complies with PyPi requirements for building a package. With this, you can either specify the dependices directly in the toml file or pin to an external file (here requirements.txt). In both cases, the toml file will rely on pip. Theres a compatibility issue with how the version are 'locked' in the requirements between pip and conda, so a common txt/yaml is not straightfoward.
Another option would be to also include an environment.yaml for conda installs, but it feels like a duplication to me and a risk to not update them both each time.
Anyways, I guess it will be a good topic to discuss on next GDL bi-weekly.
| readme = "README.md" | ||
| authors = [ | ||
| { name = "Victor Alhassan", email = "victor.alhassan@NRCan-RNCan.gc.ca" }, | ||
| { name = "Luca Romanini", email = "luca.romanini@NRCan-RNCan.gc.ca" }, |
There was a problem hiding this comment.
need to list all developers from our team who worked on that unless it's a fully new product
There was a problem hiding this comment.
I was not sure about that part because. I treated this section as 'who to contact'. As Victor pretty much did a full overhaul of the code, I thought he could be the sole author until we contribute bigger PRs, but also did not want him to have the burden to be the only person to be contacted.
There is also a maintainers tag we could use, let me know what feels best/fair for all.
LucaRom
force-pushed
the
notebook_n_base
branch
from
a7e04e0 to
f51a8ee
Compare
valhassan
left a comment
valhassan
left a comment
There was a problem hiding this comment.
Suggested some improvements to the notebook, otherwise things are looking solid for a first release.
There was a problem hiding this comment.
It's useful to show the working results for each cell. For example, the training example was terminated.
I ran the notebook cells and it all works, so best to show the working result. Also, set " accelerator="gpu" " if you can, it's way faster, I suspect you terminated early because of the slow cpu epoch runs. Lastly, add ckpt_path to use the best model when evaluating it gives way better results, bringing hope! "trainer.test(model, datamodule=dm, ckpt_path=trainer.checkpoint_callback.best_model_path)"
LucaRom
force-pushed
the
notebook_n_base
branch
from
62aeb6e to
f51a8ee
Compare
…d call the best model at test inference
3 participants
Overview
This PR implements a first notebook to demonstrate the project workflow and usage, and it shall be the last PR before merging into the main branch.
This PR also introduces several enhancements to improve usability, testing, and maintainability of the project.
Key Changes
Notebooks & Documentation
.ipynbfiles from pre-commit hooksTesting
SegmentationUnetPlus,CSVDataModule, and aTrainer.fit()run withfast_dev_runassertstatements in thetests/folderImports & Style
TYPE_CHECKING(TC002 exception)__init__file.gitignoreEnvironment & Metadata
pyproject.tomlwith detailed metadata and extended isort configurationrequirements.txtwith pinned dependenciesrequirements.txt