Remove requirements-dev.txt

[mfisher87]

Description

Follow-up from #485. It's a pain to keep pip and conda dev requirements files both up-to-date (they are currently out-of-sync), so @maresb felt it best to remove the pip one. There are more dependencies on this than expected:

• Netlify (who can edit?)
• "test" workflow
• "mkdocs" workflow

[netlify]

✅ Deploy Preview for conda-lock ready!

Name	Link
🔨 Latest commit	57588a3
🔍 Latest deploy log	https://app.netlify.com/sites/conda-lock/deploys/6529c248aa7cd20008331e69
😎 Deploy Preview	https://deploy-preview-504--conda-lock.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[mfisher87]

Hm. Not sure why the windows tests are so unhappy:

RuntimeError: Download error (23) Failed writing received data to disk/application [https://conda.anaconda.org/rapidsai/noarch/repodata.json.zst]\nFailure writing output to destination

The main environment installs OK, but this seems to fail when pytest is installing more stuff?

[mfisher87]

OSX also failing on installing dependencies during pytest run:

requests.exceptions.HTTPError: 504 Server Error: Gateway Timeout for url: https://api.anaconda.org/download/conda-forge/micromamba/1.5.1/osx-64/micromamba-1.5.1-0.tar.bz2

[mfisher87]

Looks like all the tests are OK after being re-run, thanks @maresb. Marking this ready for review. I think we could change the Netlify build command from

python -m pip install -r requirements-dev.txt \
  && python -m pip install . \
  && mkdocs build

to:

python -m pip install conda-lock \
  && conda-lock install environments/conda-lock.yml \
  && python -m pip install . \
  && mkdocs build

or, if we don't want to use the lock file (now that I'm thinking about it, this PR needs to update the lockfile 😆):

"${SHELL}" <(curl -L micro.mamba.pm/install.sh) \
  && micromamba install -f environments/dev-environment.yaml \
  && micromamba activate conda-lock-dev \
  && pip install . \
  && mkdocs build

☝️ I'm sure there are some mistakes in this.

[maresb]

@mariusvniekerk, could you please help with Netlify?

[mariusvniekerk]

Netlify supports setting the build command in a config file these days.

https://docs.netlify.com/configure-builds/file-based-configuration/#build-settings

[mfisher87]

I'm confused about 5391eee ; shouldn't the lockfile be updated alongside the dev-environment.yml?

[mfisher87]

Looks like the best way to install from the conda spec on Netlify might be to install micromamba. I expected conda might be available, or we might be able to provide another docker image to run the build, but not so.

I hoped to test locally, so I checked out the netlify/build image on Dockerhub, and that seems to be different from the one that runs the build command. The default Python install on that image is 2.7 and latest seems to be 3.7 😢 I apologize for all the force pushes, but I didn't find another effective way to test.

[mfisher87]

😅 wish GitHub would at least hide my shame in an accordion section!

I got it working, but the command is far from pretty.

[mfisher87]

@maresb ready for review!

[maresb]

Nice @mfisher87!!! It looks like the tests are running with the correct Python versions, so I think this is all good.

Also, GitHub collapsed things for me, so my view is clean. No worries.

I'm confused about 5391eee ; shouldn't the lockfile be updated alongside the dev-environment.yml?

I think he just did this to resolve a merge conflict. So yes, if you update the environment it should be relocked, but only in a separate commit at the end. You did everything right.

Actually, apart from conda-lock install in Netlify, it doesn't look like this uses the lockfile, so probably we don't need to update it in this PR, unless you want mamba for something in particular. That way we can use the normal auto-update mechanism.

Is there a particular reason for including mamba in the dev dependencies? Just to ensure that it's available during development?

Shall I squash-merge this? Alternatively you could rebase it with something like (untested!):

git checkout main
git pull
git checkout remove-pip-dev-requirements
git reset --hard main
git cherry-pick e3cf3052d4ad6493cdcd3e7a3684463d06576934
git cherry-pick b63484ed4086b908bda8bf2307960f2f2a80f2f6
git push --force

By the way, it looks like we're missing ruff in the dev dependencies. No need to do it here, but if you're editing the dev dependencies anyways, then we could just sneak it in here.

[mfisher87]

Actually, apart from conda-lock install in Netlify, it doesn't look like this uses the lockfile, so probably we don't need to update it in this PR, unless you want mamba for something in particular. That way we can use the normal auto-update mechanism.

Is there a particular reason for including mamba in the dev dependencies? Just to ensure that it's available during development?

I wish I remembered! It's been some time; I think the tests run faster by using the mamba solver when it's available? Happy to revert as well.

By the way, it looks like we're missing ruff in the dev dependencies. No need to do it here, but if you're editing the dev dependencies anyways, then we could just sneak it in here.

I think we may have had a conversation about this previously, but do you feel strongly about including Ruff there? Since pre-commit manages its own dependencies, I like how it shrinks my dev dependency lists :)

Shall I squash-merge this? Alternatively you could rebase it with something like (untested!):

I'm real comfortable with interactive rebasing, I'll take care of that shortly :)

[mfisher87]

By the way, it looks like we're missing ruff in the dev dependencies. No need to do it here, but if you're editing the dev dependencies anyways, then we could just sneak it in here.

I think we may have had a conversation about this previously, but do you feel strongly about including Ruff there? Since pre-commit manages its own dependencies, I like how it shrinks my dev dependency lists :)

@maresb If we decide we don't need to add Ruff, I'd also propose removing Mypy, Black, isort from this file.

isort should be removed either way, since we don't even use it with pre-commit anymore after #493 was merged. Woops! Once you share your thoughts on this and the mamba dev dependency I'll get these last bits tied up and ready for merge.

If you prefer, can also squash down to 1 commit with a more detailed message.

[maresb]

I wish I remembered! It's been some time; I think the tests run faster by using the mamba solver when it's available? Happy to revert as well.

Looking at this freshly, I think we simply need Mamba in the test environment for when we test conda-lock+mamba.

I think we may have had a conversation about this previously, but do you feel strongly about including Ruff there? Since pre-commit manages its own dependencies, I like how it shrinks my dev dependency lists :)

I don't feel strongly. I'm still getting accustomed to it, so I've been playing around with the CLI lately, adding it as a dev dependency so I can do that. But ruff is <5MB and has no dependencies (apart from Python), so it's extremely lightweight. Feel free to leave it out though, or we can decide later.

Thanks for the rebase! Looks nice and tidy now.

If we decide we don't need to add Ruff, I'd also propose removing Mypy, Black, isort from this file.

Yes. But since we're talking multiple packages I'd be inclined to do this in a separate PR.

If you prefer, can also squash down to 1 commit with a more detailed message.

Two commits are good. I much prefer having more commits as long as they're not being undone.

If the Windows test passes I think we could merge this now. Is it ready from your side?

[mfisher87]

Looking at this freshly, I think we simply need Mamba in the test environment for when we test conda-lock+mamba.

This agrees with my recollection! And I think the tests which are run by default are those ☝️

I felt it made sense to add Ruff and remove isort from the environment.yml (my justification is that both actions are fixing things that are out of sync with recent changes to our pre-commit config), and I re-locked.

If the Windows test passes I think we could merge this now. Is it ready from your side?

We're good to go now! 🚀

[maresb]

Hmm, I'm worried about the failing Windows test. It's passing on other PRs. Rerunning...

[mfisher87]

I haven't looked, but maybe one of the Windows tests needs to be marked as @flaky? That's happened on this PR a couple times and re-running resolved it. Thanks Ben :)