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

Add devcontainer integrations #451

Merged
merged 18 commits into from
Mar 22, 2024
Merged

Add devcontainer integrations #451

merged 18 commits into from
Mar 22, 2024

Conversation

jaimergp
Copy link
Contributor

@jaimergp jaimergp commented Mar 14, 2024

Description

Based on conda/conda#13648

Makes everything super easy ✨ in VS Code. I don't know why we didn't set this up earlier!

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Mar 14, 2024
@jaimergp jaimergp mentioned this pull request Mar 14, 2024
3 tasks
Copy link
Contributor

@zklaus zklaus left a comment

Choose a reason for hiding this comment

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

I ran into some trouble with the Mamba v1 support. Before I go down that rabbit hole, is that actually ready and working for you? After installing sudo, I run into missing cmake, ...

.devcontainer/post_start.sh Show resolved Hide resolved
.devcontainer/post_create.sh Show resolved Hide resolved
.devcontainer/post_create.sh Outdated Show resolved Hide resolved
.devcontainer/post_create.sh Outdated Show resolved Hide resolved
.devcontainer/post_start.sh Outdated Show resolved Hide resolved
@zklaus
Copy link
Contributor

zklaus commented Mar 16, 2024

Obviously I love the idea 😄. Since this requires a couple of repos, when I was thinking about this in the context of the conda devcontainers PR, I was wondering if it makes sense to create a dedicated workspace repo, with the other three repos as submodules and the overarching workspace definition along with devcontainers in the repo root.

@jaimergp
Copy link
Contributor Author

Since this requires a couple of repos, when I was thinking about this in the context of the conda devcontainers PR, I was wondering if it makes sense to create a dedicated workspace repo, with the other three repos as submodules and the overarching workspace definition along with devcontainers in the repo root.

That could be a good setup, but given how unlikely it is that we develop CLS in isolation (we will mostly test against conda, or debug conda/conda upstream tests, etc), I think for now we are good with assuming that conda/conda is expected next to conda/conda-libmamba-solver.

Before I go down that rabbit hole, is that actually ready and working for you?

Didn't try it yet. I was mostly copying fragments from different files into the new places, but then found out that it'd be better to tackle the CI renovation before this one.

I'll ping when this is ready for review, sorry for the noise!

@jaimergp jaimergp marked this pull request as draft March 18, 2024 13:27
@jaimergp
Copy link
Contributor Author

Failures due to the conda.auxlib.deprecation in conda and the fact we install conda-build from its latest release. Changing to conda-canary/label/dev works sometimes and it's very fragile so I'd say we can merge this with failures (provided the review goes ok), because they will be fixed eventually upstream.

@jaimergp jaimergp marked this pull request as ready for review March 20, 2024 11:02
@jaimergp jaimergp requested review from zklaus and jezdez and removed request for zklaus March 20, 2024 11:02
Copy link
Contributor

@zklaus zklaus left a comment

Choose a reason for hiding this comment

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

Just a few clarification requests.

.devcontainer/conda-forge/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/conda-forge/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/defaults/devcontainer.json Outdated Show resolved Hide resolved
Comment on lines 24 to 27
if [ ! -f $CONDA_SRC/pyproject.toml ]; then
echo "conda/conda not found, cloning..."
git clone https://github.com/conda/conda $CONDA_SRC
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to bail with an error. There are situations where a user may have their working directory in place but without a pyproject.toml file. In other cases, someone may have their conda sources in a different place. That will not work with this setup, of course, but then it's better for them to notice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Added.

.devcontainer/post_create.sh Outdated Show resolved Hide resolved
.devcontainer/post_create.sh Outdated Show resolved Hide resolved

set -euo pipefail

MINICONDA=${MINICONDA:-/opt/conda}
Copy link
Contributor

Choose a reason for hiding this comment

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

In l 23 we write that Miniconda is not compatible with Mamba. Would it be better to use a different variable name here, like BASE_PREFIX or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Adjusted all the env var names for clarity.

.devcontainer/conda-forge/devcontainer.json Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to remove the docker version of the setup or keep it as an alternative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't tested it manually, but I guess one could also pull from condaforge/miniforge, mount volumes and then run post_create and post_start? I'll add it as a section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

.devcontainer/post_start.sh Outdated Show resolved Hide resolved
jaimergp and others added 4 commits March 21, 2024 11:30
Co-authored-by: Klaus Zimmermann <klaus.zimmermann@quansight.com>
@jaimergp
Copy link
Contributor Author

pre-commit.ci autofix

Copy link
Contributor

@zklaus zklaus left a comment

Choose a reason for hiding this comment

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

Almost there...

.devcontainer/conda-forge/devcontainer.json Show resolved Hide resolved
.devcontainer/conda-forge/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/defaults/devcontainer.json Show resolved Hide resolved
.devcontainer/defaults/devcontainer.json Outdated Show resolved Hide resolved
jaimergp and others added 2 commits March 21, 2024 17:52
Co-authored-by: Klaus Zimmermann <klaus.zimmermann@quansight.com>
Co-authored-by: Klaus Zimmermann <klaus.zimmermann@quansight.com>
Copy link
Contributor

@zklaus zklaus left a comment

Choose a reason for hiding this comment

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

Great 🎉

@jaimergp jaimergp merged commit 1f4fccb into main Mar 22, 2024
62 of 65 checks passed
@jaimergp jaimergp deleted the devcontainer branch March 22, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants