Improved packaging and CI pipeline for lighter

[surajpaib]

No description provided.

[ibro45]

I see you included #14, but that hasn't been merged yet, just a heads up. I will get back to that PR today or tomorrow.

[surajpaib]

@ibro45 Work in progress for now, will merge all effective branches when this goes in at the end

[surajpaib]

Before I merge this, it would be great if you both @ibro45 @kbressem could test the following,

1. Installing lighter in a new environment using this packaging update.
2. Setting up the pre-commit hook and checking if everything works as expected. (Maybe make a few dummy commits with README.md)
3. Adding a new dependency to the project using poetry add <dependency>. Both of you can try a different dependency, a lock file is maintained of the exact versions, I want to be sure that it resolves correctly when another user tries it out. Make sure to do poetry lock after you add the dependency.

I will resolve the errors for the builds and we can also check if the tests run as expected.

[kbressem]

Installation with poetry worked fine. I can import lighter in python, although including the version in the __init__.py would be good.

However, I get a lot of mypy errors directly after make install. It seems this is done automatically with the installation?

[ibro45]

I discussed with Suraj in private about this, we found a solution, but @kbressem did you not have it?

[surajpaib]

@ibro45 So the keyring happens only on certain systems where there is secure keyring installed. If you tested it on our machines in Boston, you should have this issue. But not all systems have this, for example, machines running the CI do not have this issue.

@kbressem Will add version to the init! Thanks for pointing out. I also think mypy should be out of the install loop, fixing

[kbressem]

I have also tried adding rich (awesome library btw.) as dependency. Seemed like it worked just fine.

Tests are passing as well, after I've added the dependency.

Removing rich worked well, too.

[surajpaib]

@Keno The version string should be available now. We can bump versions with the command,
poetry version patch. Add --dry-run to view demo.

@ibro45 The console scripts does not work as expected yet, doing make install will install the script but then you need to run it with poetry run <script> until we deploy

[surajpaib]

@ibro45 lighter cli interface works now.

Will wait for you to merge the logger and I can re-submit this for review to be merged then.

Ideally we need someone to test this in a fresh environment and see if all functionality works as expected

[surajpaib]

@kbressem @ibro45

Ready for review and verification from your side to merge.

I've added an integration test as well to check if something would break on PRs
https://github.com/project-lighter/lighter/blob/packaging/tests/integration/test_cifar.py

Very simple test for now, will add more functionality and clean it up once we have this addressed Project-MONAI/MONAI#5899

I will create another issue for a potential suite of tests

[kbressem]

To be honest, I do not even understand half of the poetry configs, so I cannot check if everything is correct. But overall it looks really good and all configurations seem reasonable and well thought through.

I will install the library again in a fresh environment and then finish this review.

[kbressem]

To reduce testing time, we should simplify the tests and only focus on testing if the code runs.

Additionally, we could implement slower tests that run for multiple epochs with a fixed seed. These tests will ensure that the metrics remain consistent, indicating that the code is functioning correctly and the network trains correctly. But such a test will take several minutes and is annoying if you want to just add a simple PR.

Suggested change

	trainer#max_steps: 5
	trainer#num_sanity_val_steps: 0
	trainer#accelerator: cpu
	system#batch_size: 16
	system#num_workers: 2
	trainer#callbacks: null
	trainer#max_steps: 2
	trainer#num_sanity_val_steps: 0
	trainer#accelerator: cpu
	system#batch_size: 4
	system#num_workers: 2
	trainer#callbacks: null

[surajpaib]

I agree! We should have two sets of tests. One that runs by default on PR and then another that we can trigger manually when we want to merge more breaking changes (this will be the longer multiple epoch ones).

For the latter, pytest provides some nice functionality to tag tests with different markers. If you see the integration test here:

lighter/tests/integration/test_cifar.py

Line 13 in 5d02775

@pytest.mark.slow

I've marked it as slow. We can set pytest markers for each of the two sets accordingly and run them as we need

[surajpaib]

To be honest, I do not even understand half of the poetry configs, so I cannot check if everything is correct. But overall it looks really good and all configurations seem reasonable and well thought through.

I will install the library again in a fresh environment and then finish this review.

I am also still getting used to poetry but having someone test the functionality is what we need at this point. I think we can figure deeper concepts as we go then

[ibro45]

Test: #24

Everything went smoothly.

Approving, and a reminder for PyPi.

[ibro45]

make lint - mypy still goes through my other folders under projects/

[surajpaib]

make lint - mypy still goes through my other folders under projects/

Do we want it to skip all of projects? or only do it on cifar10?

[surajpaib]

Test: #24

Everything went smoothly.

Approving, and a reminder for PyPi.

Amazing! Will wait for @kbressem also to test and revert since this a major change for the repo

[ibro45]

Sounds good!

I'd skip everything except the folders that we use for demos, right now it's only cifar10

[kbressem]

unfortunately I now run into an error with make setup:

[ibro45]

@surajpaib seems like exec bash is messing up with his zsh too

[kbressem]

@surajpaib seems like exec bash is messing up with his zsh too

Yes. Just removing it worked for me (see also #24). Also, I had to first uninstall poetry before I could reinstall it. Maybe we can add poetry-remove as first command in make setup to remove any old installations.

[surajpaib]

@ibro45 @kbressem

Removing exec bash will work on all shells without an issue if you don't have a secure keyring enabled.

Poetry has an issue where it breaks if the secure keyring is enabled.
python-poetry/poetry#1917

I've tried to use the fix mentioned in the issue. For now, it only works for bash. We'll need to see what to do if people have different shells and have the keyring enabled. I'll work on a long-term fix for our repo for this. But maybe in a different issue.

@kbressem Agree with adding poetry remove! Will do so. Maybe even a full cleanup so everytime setup is run, its a fresh install

[ibro45]

Just something that came to my mind, gotta check it later, but writing it here not to forget - when cloning lighter you also need to run a command to pull lighter.contrib submodule - is that done automatically when installing with poetry?

[ibro45]

Just something that came to my mind, gotta check it later, but writing it here not to forget - when cloning lighter you also need to run a command to pull lighter.contrib submodule - is that done automatically when installing with poetry?

Actually, it'd make more sense if it's another package that'd be installed like pip install project-lighter-contrib, and found at lighter.contrib. In that case we remove contrib submodule from lighter core. Something like https://pypi.org/project/opencv-contrib-python/ and the archived https://github.com/pytorch/contrib