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

Update infrastructure #507

Merged
merged 26 commits into from
Jul 30, 2024
Merged

Update infrastructure #507

merged 26 commits into from
Jul 30, 2024

Conversation

timmens
Copy link
Member

@timmens timmens commented Jul 21, 2024

  • Use hatch for packaging
  • Add infrastructure to release both estimagic and optimagic
  • Use mypy as pre-commit
  • Replace black and nbqa by ruff
  • Fix ruff warnings
  • Add token PYPI_API_TOKEN_OPTIMAGIC as secret to repo

Open Questions:

  1. Should we rename the old token secret from PYPI_API_TOKEN to PYPI_API_TOKEN_ESTIMAGIC. In this case we would need to create a new one.
    -> No
  2. Find a better place than the new folder .scripts to store the python file update-name-in-pyproject.py. Proposal: Combine contents of .scripts folder with .envs folder in new folder called .ci.
    -> We will use .tools for everything now

mypy discussion

For now, I have opted to keep both the mypy pre-commit and the mypy GitHub Action task. Given tools like pre-commit CI, I see an advantage in not using the development environment for the hooks. However, since we are not entirely sure that this mypy call is doing the same thing that the mypy call from our dev environment is doing, I believe the GitHub Action task is necessary. Combined, we should (1) be able to gain some experience with pre-commit mypy, (2) ideally get quick errors before committing, and (3) be confident that all errors should be caught, at least after pushing.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@janosg
Copy link
Member

janosg commented Jul 22, 2024

I sometimes get this warning when running pre-commits. Maybe you can fix it with your PR

warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'extend-ignore' -> 'lint.extend-ignore'
  - 'select' -> 'lint.select'
  - 'pydocstyle' -> 'lint.pydocstyle'
  - 'per-file-ignores' -> 'lint.per-file-ignores'

@timmens timmens requested a review from janosg July 25, 2024 13:33
@janosg
Copy link
Member

janosg commented Jul 28, 2024

  1. Should we rename the old token secret from PYPI_API_TOKEN to PYPI_API_TOKEN_ESTIMAGIC. In this case we would need to create a new one.

No, having PYPI_API_TOKEN and PYPI_API_TOKEN_OPTIMAGIC is clear enough and we don't need to spend time on the renaming.

Find a better place than the new folder .scripts to store the python file update-name-in-pyproject.py. Proposal: Combine contents of .scripts folder with .envs folder in new folder called .ci.

Yes, we should combine all pre-commit hooks in one folder but the resulting environment files should still stay in .envs. .ci is not a good name because we will soon have a lot more pre-commit hooks that are actually code generation tools and not related to quality assurance (see here for details). We might also have related pytest hooks. I am almost indifferent between the names .scripts and .tools but would not get more specific.

Copy link
Member

@janosg janosg left a comment

Choose a reason for hiding this comment

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

Looks very good. Thanks. Just minor comments.

.github/workflows/publish-to-pypi.yml Outdated Show resolved Hide resolved
docs/source/development/ep-02-typing.md Show resolved Hide resolved
environment.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@timmens
Copy link
Member Author

timmens commented Jul 29, 2024

Warning looks like this:

image

src/estimagic/__init__.py Outdated Show resolved Hide resolved
src/optimagic/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@janosg janosg left a comment

Choose a reason for hiding this comment

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

Thanks, very nice PR!

@timmens timmens merged commit 03ec822 into 0.5.0 Jul 30, 2024
14 checks passed
@timmens timmens deleted the infrastructure branch July 30, 2024 09:10
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