Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to Monai Bundle config #10

Merged
merged 11 commits into from
Jan 17, 2023
Merged

Switch to Monai Bundle config #10

merged 11 commits into from
Jan 17, 2023

Conversation

ibro45
Copy link
Collaborator

@ibro45 ibro45 commented Jan 14, 2023

Closes #7.

Still needs to be addressed:

Note:

  • Calling lighter fit --config_file ... is basically equal to python -m monai.bundle run fit --config_file ....

Conclusion:

@ibro45 ibro45 marked this pull request as draft January 14, 2023 06:46
@ibro45
Copy link
Collaborator Author

ibro45 commented Jan 16, 2023

@surajpaib Cannot seem to make the user project import work, even though the same implementation worked before monai bundle config

@surajpaib surajpaib marked this pull request as ready for review January 17, 2023 00:54
@surajpaib
Copy link
Collaborator

I think I understand what the issue is, the project function is only instantiated, but never called

@surajpaib
Copy link
Collaborator

surajpaib commented Jan 17, 2023

Here is a working solution for your previous approach

project:
    _target_: lighter.utils.import_module_from_path
    module_name: project
    module_path: ./projects/cifar10

trainer:
    _target_: pytorch_lightning.Trainer
    max_epochs: 50
    accelerator: gpu
    devices: 1

system:
    _target_: lighter.LighterSystem
    batch_size: 512
    pin_memory: True
    num_workers: 4
    log_input_as: "image_batch"

    model:
        _target_: project.models.net.Net
        _requires_: "$@project()"

@surajpaib
Copy link
Collaborator

@ibro45 I'm thinking of a better way to do this overall, will post when I have an update

@ibro45
Copy link
Collaborator Author

ibro45 commented Jan 17, 2023

Sorry, didn't see your comments. So, in the meantime i realized we don't need the import project. That makes it a bit better. In your example I have to say that I don't like having to call the partial function like _requires_: "$@project()". Actually, I've been confused by why they didn't implement it so that it's called whenever you refer it, without all the $ and (). Let's see what happens with that PR they opened, I'll suggest it if they don't end up doing it.

@surajpaib
Copy link
Collaborator

I get why they did it this way though because you want a reference to the function and not call the function itself in many cases. And then be able to call it into your code however you want.

For example. if you defined a train, test and val functions, and then want to call them accordingly.
I think its difficult to achieve both functionality together though :(

@ibro45
Copy link
Collaborator Author

ibro45 commented Jan 17, 2023

Managed to get it to behave as it did before monai bundle config. Ready to be merged if you agree.

Copy link
Collaborator

@surajpaib surajpaib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing stuff with monai-bundle! Looks good to go, added a comment about a small refactor, let me know if you think it makes sense and I can add a commit to the PR

lighter/cli.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@surajpaib surajpaib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go from me!

@surajpaib surajpaib merged commit 7ec2112 into main Jan 17, 2023
@surajpaib surajpaib deleted the monai-bundle branch January 17, 2023 14:49
surajpaib added a commit that referenced this pull request Jan 17, 2023
@surajpaib surajpaib restored the monai-bundle branch January 17, 2023 14:54
surajpaib pushed a commit that referenced this pull request Jan 17, 2023
- switch to monai bundle config
- move yaml load function for readability
- user project import not working
- Functional user project importing, but ugly af
- unnecessary import statement
- Updated project import with class def
- import user project without yaml imports
- Cleaned up cli

Co-authored-by: Suraj Pai [bspai@bwh.harvard.edu](mailto:bspai@bwh.harvard.edu)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace Hydra with monai bundle
2 participants