Skip to content

Conversation

hectormz
Copy link
Contributor

This PR reorders the conda environment files across operating systems to make them easier to maintain (and separates core and dev dependencies). It also adds Windows Python 3.9 env file.

@canyon289
Copy link
Member

Thanks @hectormz!

@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

Merging #5895 (eca3f79) into main (0ba47b5) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5895   +/-   ##
=======================================
  Coverage   89.48%   89.49%           
=======================================
  Files          73       73           
  Lines       13275    13275           
=======================================
+ Hits        11879    11880    +1     
+ Misses       1396     1395    -1     
Impacted Files Coverage Δ
pymc/step_methods/hmc/base_hmc.py 90.47% <0.00%> (+0.79%) ⬆️

@hectormz hectormz changed the title Align conda envs and add Windwos 3.9 env Align conda envs and add Windows 3.9 env Jun 14, 2022
@michaelosthege
Copy link
Member

I like that the environment files with the comments are easier to understand, but we have a pre-commit check that automatically sorts them.
@hectormz could you check how that pre-commit hook works and if it could maybe sort only within each comment section?

If that's too complicated (very likely) we should discuss if we want to remove the automated sorting.

Personally, I'd be okay with that, since on all of my (Windows) machines that pre-commit hook always and annoyingly creates an empty line in the Windows env files that mustn't be committed.

@hectormz
Copy link
Contributor Author

@michaelosthege Ah, forgot to install pre-commit in my new environment. The hook in its current form isn't sorting all the .yml in the folder anyway. I'll look into it. I also checked out sort-simple-yaml hook, but it's not simple enough

@michaelosthege
Copy link
Member

@hectormz I asked folks on Slack and we'd be ok with removing the env sorting step from the pre-commit in favor of the explanatory comments you made here.

So feel free to do that commit and just let me know when the pre-commit passes locally, then I'll approve running the tests. (someone has to click this button every time you push..)

If you have other things on the schedule just let us know and someone can take over 🙂

@hectormz
Copy link
Contributor Author

Thanks for the update @michaelosthege , just pushed! I also updated the script that generates pip requirements from conda env because it makes extra empty lines on Windows due to line ending.

@hectormz
Copy link
Contributor Author

Did you want to add windows-environment-dev-py39.yml to the tests.yml GitHub action?

@michaelosthege
Copy link
Member

Did you want to add windows-environment-dev-py39.yml to the tests.yml GitHub action?

You mean to run the tests in that environment? In principle yes, but we'll want to make sure that we're not running any tests twice.
Also, there's another open PR right now that's about changing which Python versions the CI runs with. Please check to avoid git conflicts or opposing momentum

@hectormz
Copy link
Contributor Author

Hi @michaelosthege no need to make those changes here. I was just wondering out loud. I'm content to leave this PR focused on the changes made.

Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@canyon289 can you give it a final look and hit the merge button if you're happy?

@ricardoV94
Copy link
Member

We will hopefully remove python version specific versions in #5911, but merging this already! Thanks for your help @hectormz

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.

4 participants