✨ Add devcontainer

[julien-blanchon]

Add .devcontainer directory with basic working devcontainer

[adamjstewart]

What is this and why would we want it?

[calebrob6]

https://docs.github.com/en/codespaces/setting-up-your-project-for-codespaces/adding-a-dev-container-configuration/introduction-to-dev-containers

[adamjstewart]

How is this different from #367/#368 which was rejected?

[julien-blanchon]

Hi, devcontainer make it easier to contribute, basically you just define the what's the basic/minimal development system (here ubuntu+python and requirements), and open source contributor could directly launch the repo on this predefine system.

The usual usage are:

• Open VS-Code with devcontainer extensions installed, user click on open in devcontainer and the VS-Code launch in the devcontainer system with all requirements installed.
• Open on cloud provider such as codespace/gitpod ... The cloud provider will directly read the devcontainer in order to repo with in the dev system.

So it's kind like your conda env, redondent with ./requirements but help a lot in the user/dev perspective.

It's up to you, I will at least keep it on my branch as it help me standardize my development system

[adamjstewart]

I've never heard of or used any of these things before, so I guess it's difficult for me to want to maintain these files. I've at least used pip/conda/spack before, so I don't mind supporting those installation methods. But any file we add to this git repo is something we're committing to maintain, and I'm trying to keep that burden minimal.

Like the docker PR, this feels very specific (what if someone wants to use RHEL instead of Ubuntu? or macOS or Windows? or Python 3.8?), and we don't want to maintain a file like this for every system and Python version. Maybe we could just add this file to .gitignore so you can keep it in your local copy without worrying about it getting sucked into PRs?

At least it doesn't duplicate the list of places I need to update dependencies. I would like to get rid of requirements/ but it's needed for CI. If I could, I would delete environment.yml too but I think @calebrob6 is using it 😄

So this isn't a no, but I'm not yet convinced.

[calebrob6]

@julien-blanchon thanks for the contribution! I've never used Codespaces before, is the idea that if you open microsoft/torchgeo (or a fork of it) in a Codespace environment, then it will automatically run these files?

@adamjstewart I don't think this is a huge overhead (similar in scope to the pre-commit hook) and generally makes it easier for some people to edit. The only thing we would need to change is what is pip installed. I also don't think it is some slippery slope either -- if someone doesn't want to use it then they don't have and if someone wants it to be different then they can open a PR.

[adamjstewart]

similar in scope to the pre-commit hook

I want to get rid of this too 😆

[calebrob6]

Why -- you don't have to maintain it? (put differently, these things don't get released / tested -- if they break they are going to break for technical people that are already trying to contribute and who will tell us about it)

Is there a like this nagging feeling that you get when something isn't under the comforting umbrella of pytest 🙂?

[adamjstewart]

you don't have to maintain it?

Someone has to maintain it. precommit becomes quickly out of date any time a style or mypy dep changes. I just had to upgrade the black version the other day.

Is there a like this nagging feeling that you get when something isn't under the comforting umbrella of pytest 🙂?

Yes 🙂

[calebrob6]

Yeah but that's only because you have an insane memory for these things. If you forgot, then nothing bad would have happened and ashnair would have gotten to it (https://github.com/microsoft/torchgeo/commits/main/.pre-commit-config.yaml). If ashnair didn't get to it then someone else would have.

[calebrob6]

(@julien-blanchon sorry for meta-discussion here, do you mind signing the CLA?)

[julien-blanchon]

Yes sure,

@julien-blanchon please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

• (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.

@microsoft-github-policy-service agree

• (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.

@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

[julien-blanchon]

Here is a little example on the vscode desktop app. It should also work with Intellij, expect for the extensions part.

Screen.Recording.2023-02-04.at.02.03.47.mov

CPU and GPU configuration must be split, at least for now.

About codespace, here is the setup example:

The image build could be quite long, expecially with codespace. So you can enable image prebuild in "settings/codespaces/prebuild_configurations/new". So any contributor will be able to launch the project with the specify configuration (same extensions, same black/mypy/... conf ...)

[adamjstewart]

Still not convinced that this is something we want to maintain (why stop at codespaces when there are dozens of other editors and environments with dotfiles we could add?) but figured I would comment on some of the issues I see.

[adamjstewart]

I'm slowly warming up to the idea of adding this. Other repos (nbmake) also ship devcontainers, and it can make it easier to reproduce bugs.

[calebrob6]

@adamjstewart I'd like to merge this, I don't see any of the open issues as blockers and it makes it easier to for people to contribute to torchgeo

[adamjstewart]

I see all of the above as blockers, shouldn't be hard to investigate these.

[julien-blanchon]

Here is some improvement we can do in term of .devcontainer:

• Remove gdal manual installation and let pip install it Not working
• Add/Remove VSCode specific extensions for a unified experience: For community project like this one the best is to keep extensions as minimal as possible, and only ship extensions that are part of the formatting/linting process such as black, isort ...
• Validate that the formatting/linting process work as espected. I've do my best to match your specific configuration for black, pylint, flake8 ... Still TODO
• Add pip install -e . of torchgeo in postStartCommand, so the current torchgeo codebase will be install at the start of the container. Note that requirements library has already been install in the container.

[adamjstewart]

@julien-blanchon are you still working on this? Would love to get this merged someday so we can use codespaces for PR reviews.

[julien-blanchon]

@adamjstewart Could you have a look ! It's way simpler than before and work both on CPU and GPU

[julien-blanchon]

Look like almost all the test pass inside the devcontainer expect some dataset and transform ones. I'm not familiar enouth with torchgeo. Could you have a look into that before merging ?

[adamjstewart]

Do we need these? Not sure if I want to enforce any style constraints that aren't already enforced in CI.

[julien-blanchon]

That's extension, I would keep them espectially pylance. This give many IDE feedback to the user even before the CI. For example the current settings will display pylint error on the IDE side before any CI. Black will also auto format the code on file save with the same specification as the CI. This may put less work on the CI side and enforce best practice directly in the IDE

[adamjstewart]

Yes, I definitely want to keep black/mypy/isort/pydocstyle/flake8 because we also enforce that in CI. I'm specifically commenting on things like pylance and pylint which we don't use.

[julien-blanchon]

You can remove pylint. I would keep pylance anyway because it's just visual, it doesn't enforce anything, but feel free to remove it if you'r sure you don't need it
Btw for torchgeo in general you can have a look to ruff that bundle everything in a single package.

[adamjstewart]

See #1908 (comment) for my thoughts on ruff. Not strongly opposed in principle, but let's discuss that in a separate issue.

[adamjstewart]

Does pylance flag anything that the other tools don't flag?

[adamjstewart]

Don't know much about this, but someday I'll learn how to use Codespaces and VS Code.

[adamjstewart]

Can you tell me more about the failing tests? Do those tests fail for you when manually run in your local environment, or do they only fail in VS Code?

[julien-blanchon]

Can you tell me more about the failing tests? Do those tests fail for you when manually run in your local environment, or do they only fail in VS Code?

Seems to be due to the Python version. I think it's gone with the 3.12. I'm looking into that

[adamjstewart]

That's still odd, since we test Python 3.9–3.12 in CI and all pass.

[adamjstewart]

Let's merge as is. We can always make improvements in future PRs. Want to squeeze this in before our next release.

[julien-blanchon]

Let's merge as is. We can always make improvements in future PRs. Want to squeeze this in before our next release.

I'm going to investigate next week about the tests failed. I don't think it's blocking anyway