Skip to content

Conversation

@gnzng
Copy link
Collaborator

@gnzng gnzng commented Feb 26, 2025

This PR adds a github workflow for setting up the environment as well as running the pytest for every push and pull into master from python 3.8 - 3.12. Unfortunatelly, 3.7 is not widely supported (anymore).

Screenshot 2025-02-25 at 17 45 03

It is all running on pip, which I think is a better way forward, than using conda. pytorch doesn't even support installation via conda anymore: https://pytorch.org/get-started/locally/. If that is good way forward we could probably change the docs and update the installation.

gnzng and others added 2 commits February 25, 2025 16:04
@gnzng gnzng requested a review from allevitan February 26, 2025 01:12
@allevitan
Copy link
Collaborator

This looks good, but I want to check that the updated requirements.txt file works with the existing installation instructions.

Actually, this might be a good time to update the documentation for installation, now that pytorch no longer supports conda. I'd like to pull these two changes in together, so that the documentation stays current with the codebase.

@allevitan
Copy link
Collaborator

Haha, apparently I should not try to review a code change so soon after waking up. Let me revise what I said to "seems perfect, but could you update the installation info in the docs and add it to this pull request?"

If you don't have the docs set up, feel free to edit in markdown and add it to the PR - I can test building the docs and then push a final version.

@gnzng
Copy link
Collaborator Author

gnzng commented Feb 26, 2025

Just opened another PR over at cdtools-developers/cdtools-docs#1

@gnzng
Copy link
Collaborator Author

gnzng commented Feb 26, 2025

I added a demonstration on how that would look like in a PR over in my own cdtools fork: gnzng#1
Screenshot 2025-02-26 at 08 06 12

run: |
pip install --upgrade pip
pip install -r requirements.txt
pip install -e . --no-deps
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we don't need to install with -e (which, I believe will go away eventually), and we might not even need to to do the pre-install of requirements.txt. But this script should work, no need to change.

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 do agree. I just wanted to implement an example, which is very close to your current installation workflow. In general I see a lot installations using the pyproject.toml, which allow you to install the package with a certain subset of packages, eg. pip install .[docs]. Here is an example on an unreleased project build on this cookiecutter skeleton: https://github.com/audreyfeldroy/cookiecutter-pypackage

dependencies = [
  "matplotlib",
  "numpy",
  "torch",
  "loguru",
  "h5py"
]

[project.optional-dependencies]
docs = [
    "sphinx",
    "sphinx-markdown-tables", 
    "numpydoc",
    "sphinx_copybutton",
    "myst_parser",
    "sphinx_rtd_theme",
    "sphinx_rtd_dark_mode"
]
tests = [
    "coverage",
    "coveralls",
    "codecov",
    "pylint",
    "pytest-cov"
]
dev = [
    "mypy",
    "pytest",
    "ruff"
]

Copy link
Collaborator

@allevitan allevitan left a comment

Choose a reason for hiding this comment

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

I think this all looks good and is a clear improvement. Let's set it up and see what happens.

@allevitan allevitan merged commit db2450c into cdtools-developers:master Feb 27, 2025
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.

2 participants