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

Deps: make omegaconf/tensorboard optional #1214

Merged
merged 4 commits into from
Apr 5, 2023
Merged

Deps: make omegaconf/tensorboard optional #1214

merged 4 commits into from
Apr 5, 2023

Conversation

adamjstewart
Copy link
Collaborator

Why optional?

TorchGeo does not import or require either of these dependencies. The only thing these deps are required for is our trainer tests, for a single notebook, and for train.py and friends. None of these files get installed when you run pip install torchgeo, so the dependency shouldn't be required.

Why now?

Omegaconf was always an issue (every time you update the conda-forge recipe it complains that it isn't used), but the lightning/tensorboard issue is relatively new. Starting with lightning 1.9, tensorboard was moved from a required dependency to an optional extra. However, our tests need tensorboard for coverage, so we added a dependency on lightning[extra] to pull in tensorboard. However, this pulls in a lot of other deps in addition to tensorboard. To minimize the deps we install, we should avoid using [extra].

Also, Spack has trouble concretizing py-lightning+extra due to conflicting setuptools versions required.

Why tensorboard and not tensorboardX?

Lightning used to install tensorboard, then switched to tensorboardX (both are compatible with the same API). The reason for this switch was that tensorboardX has fewer dependencies and is easier to install on more platforms. The only other difference I've seen between the two that affects us is that tensorboardX does not support the %load_ext tensorboard Jupyter magic that we use in our notebook. Since our notebook tests require tensorboard, I figured we should just go with that.

FYI @bugraaldal

@adamjstewart adamjstewart added this to the 0.4.1 milestone Apr 1, 2023
@github-actions github-actions bot added the dependencies Packaging and dependencies label Apr 1, 2023
@github-actions github-actions bot added the testing Continuous integration testing label Apr 2, 2023
@@ -72,7 +72,7 @@ jobs:
- name: Install pip dependencies
if: steps.cache.outputs.cache-hit != 'true'
run: |
pip install .[docs,tests] planetary_computer pystac pytest-rerunfailures tensorboard
pip install .[docs,tests] planetary_computer pystac pytest-rerunfailures
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The rest of these are things that we could subsume into [tests], but they're also dependencies that I would like to remove in the future, and I don't think they're worth tracking.

@calebrob6
Copy link
Member

calebrob6 commented Apr 5, 2023

I think having tensorboard inside the notebooks isn't necessary (this is why I used CSV logger at one point). So removing those and depending on tensorboardX is fine with me. Put differently, tensorboard in notebooks isn't really necessary for showing people the features of torchgeo.

calebrob6
calebrob6 previously approved these changes Apr 5, 2023
@adamjstewart
Copy link
Collaborator Author

You should try the new trainers tutorial, it's beautiful. The CSV logger is an extra 20 lines of code and makes it seem like using TorchGeo is way harder than it actually is. No user should ever have to manually parse a CSV file and make their own loss/accuracy plots.

@calebrob6
Copy link
Member

I get that it looks cool :), but notebooks aren't really the environment for running organized experiments. In my mind the tutorials are to show off what torchgeo can do, the point of showing the loss at all is "it goes down, training is working" not showing the logging capabilities of lightning

@adamjstewart
Copy link
Collaborator Author

For sure, but the point of the tutorials is also not to teach you how to parse and plot a CSV file. The tutorials should be the bare minimum lines of code. The previous tutorial had more lines for plotting than it did for training. Now plotting is a single line of tensorboard.

@calebrob6
Copy link
Member

(but, as a consequence, brings in more dependencies)

@calebrob6
Copy link
Member

calebrob6 commented Apr 5, 2023

Also, I don't care one way or the other, just suggesting a path for using tensorboardX

(also, we could probably plot from CSV with fewer lines if that's the metric -- I love a good code golf problem ;))

@adamjstewart
Copy link
Collaborator Author

I might open an issue with tensorboardX and see if they have any interest in Jupyter support. That project is confusing to me, it seems crazy to try to maintain feature parity with such a large library as tensorboard.

@adamjstewart adamjstewart merged commit 8e5ef46 into main Apr 5, 2023
@adamjstewart adamjstewart deleted the deps/optional branch April 5, 2023 17:27
calebrob6 pushed a commit that referenced this pull request Apr 10, 2023
* Deps: make omegaconf/tensorboard optional

* Bump minimum tensorboard

* Remove explicit tensorboard install from GitHub Actions

* Fix typo
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* Deps: make omegaconf/tensorboard optional

* Bump minimum tensorboard

* Remove explicit tensorboard install from GitHub Actions

* Fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Packaging and dependencies testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants