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

chore: add new nox sessions to deal with requirements and venvs #13

Merged
merged 10 commits into from
Jun 17, 2024

Conversation

egparedes
Copy link
Collaborator

@egparedes egparedes commented Jun 12, 2024

Add:

  • New venv-3xx nox session to automatically create and setup development environments
  • New requirements nox session to automatically freeze and synchronize versions of requirements for development and testing across tools
  • requirements folder with different sets of requirements for development

Change:

  • Update development instructions
  • Update documentation-related nox sessions
  • Update minimum required dependency for dace

Remove:

  • Old requirements-dev.txt file is now a preset inside the requirements folder

Copy link
Contributor

@philip-paul-mueller philip-paul-mueller left a comment

Choose a reason for hiding this comment

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

  • Missing PR description.
  • Tests fail.

Copy link
Contributor

@philip-paul-mueller philip-paul-mueller left a comment

Choose a reason for hiding this comment

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

There is one point I would suggest you to reconsider, but it is not a blocking per se, thus I approve the PR.

noxfile.py Outdated
@nox.session(python=["3.10", "3.11", "3.12"])
def venv(session: nox.Session) -> None:
"""
Sets up a Python development environment. Use as: `nox -s venv -- [dest_path] [req_preset]
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 you should swap req_preset with dest_path, and integrate the requirement in the path of the venv.
Because in most cases you want just to select an environment, but now wou have to specify both.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The decision on the order of the arguments was on purpose to force the user to give a non-standard venv a different name. I added the requirements preset to the name of the venv now so the order can be safely changed, but the default name of the generated venvs are now more verbose, so it's not exactly a win-win, but I guess it's ok...

@egparedes egparedes changed the title chore: add tool to sync and freeze requirements across tools chore: add new nox sessions to deal with requirements and venvs Jun 17, 2024
@philip-paul-mueller
Copy link
Contributor

LGTM.

@egparedes egparedes merged commit ca03fbd into GridTools:main Jun 17, 2024
7 checks passed
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