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

Move to a conda only install of pymc (local dev workflow) #304

Merged
merged 11 commits into from
May 6, 2024

Conversation

drbenvincent
Copy link
Collaborator

@drbenvincent drbenvincent commented Mar 5, 2024

@drbenvincent drbenvincent added the devops DevOps related label Mar 5, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@drbenvincent
Copy link
Collaborator Author

Looks like the ci workflow is failing. I'm not strong on GitHub actions (see ci.yml I believe), so would appreciate any pointers. But it looks like the changes have broken the remote environment setup, which I suspected would happen :)

pyproject.toml Outdated
"statsmodels",
"xarray>=v2022.11.0",
]
dependencies = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the problem comes because you are deleting these dependencies and you are pip installing the package in the CI

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not that experienced with GitHub workflows. What am I looking for? Are you referring to the environment-file: in the with: block?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean concretely: https://github.com/pymc-devs/pymc-experimental/blob/main/.github/workflows/test.yml#L16-#L70 Here they are installing the conda env and installing the library. However, I see that they have a setup.py so this might not apply to this case 🙈

How I see it environment.yml is a specification of an environment but you still need to indicate de package dependencies and I think this is why the tests are failing unless you keep the dependencies = [ ... ] in the pyproject.toml. Maybe @maresb has a hint on how to make this cleaner?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, I'm on vacation now, but we should discuss (and update) #281.

As for Conda vs pyproject dependencies, ideally we keep both. Pyproject dependencies define the python-only dependencies that you have. On the other hand, Conda takes care of both Python and non-Python dependencies.

I haven't read carefully any of the details, so I may be missing stuff, but the way I like to handle this is to:

  1. Create a Conda environment that satisfies all the dependencies, Python or otherwise
  2. pip install the project into the Conda environment with the --no-dependencies flag so that pip installs only the project
  3. Run pip check to ensure all of the project's Python dependencies are satisfied within the Conda environment. This will trip on the case when you declare a Python dependency in pyproject.toml that is missing from your environment.yaml file. I don't really know of any tests that go in the other direction, and I don't know that it would even make sense.

Note that this means that your primary source of truth for a project's dependencies should be the pyproject.toml file, not the environment.yaml file! I think this is how it should be. It enables masochistic "experts" to do an unsupported pip install if they are so inclined.

Regarding #281, conda-lock can parse the pyproject.toml for the requirements and generate a lockfile, so apart from specifying special requirements, we may be able to replace the environment.yml with a lockfile.

@drbenvincent
Copy link
Collaborator Author

Just updated requirement to pymc>=5.14.0, in order to keep up with #325

@drbenvincent drbenvincent mentioned this pull request May 6, 2024
@drbenvincent
Copy link
Collaborator Author

When building the environment (locally) using the methods in this PR (i.e. conda):

  • all tests pass
  • 12 doctests fail

That suggests that it's not a pip/conda environment construction issue

We also get 12 failing doctests in the remove tests.

@drbenvincent drbenvincent mentioned this pull request May 6, 2024
Copy link

codecov bot commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.27%. Comparing base (9169718) to head (2bf31e1).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #304   +/-   ##
=======================================
  Coverage   77.27%   77.27%           
=======================================
  Files          21       21           
  Lines        1395     1395           
=======================================
  Hits         1078     1078           
  Misses        317      317           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@drbenvincent
Copy link
Collaborator Author

Would be good to get a sanity check to ensure I won't break everything before we merge this :)

@maresb
Copy link
Contributor

maresb commented May 6, 2024

At a glance this looks sound, but this seems to be completely user-facing, meaning it doesn't seem like anything is being validated/tested in the CI.

@maresb
Copy link
Contributor

maresb commented May 6, 2024

(At worst you're breaking something minor in the development workflow.)

@maresb
Copy link
Contributor

maresb commented May 6, 2024

On the CI front, it would be nice to move forward on #281.

@drbenvincent
Copy link
Collaborator Author

Yes, this one is user-facing. Will try to progress the ci PR very soon.

@drbenvincent drbenvincent merged commit 5cac5b3 into main May 6, 2024
7 checks passed
@drbenvincent drbenvincent deleted the some-devops branch May 6, 2024 18:54
@drbenvincent drbenvincent changed the title Move to a conda only install of pymc Move to a conda only install of pymc (local dev workflow) May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops DevOps related
Projects
None yet
4 participants