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

adding pyproject / isort #163

Merged
merged 9 commits into from Jan 12, 2023
Merged

Conversation

miili
Copy link
Contributor

@miili miili commented Jan 10, 2023

  • removed requirements.txt
  • removed bumpversion
  • added pyproject.toml
  • added setuptools_scm for version tagging
  • updated pre-commit-config.yaml
    • added isort
    • added checks for yaml and json
    • added generic tests

CI

  • added pre-commit-action
  • unified two actions to single lint action

@yetinam yetinam self-assigned this Jan 10, 2023
@yetinam yetinam added the enhancement New feature or request label Jan 10, 2023
@yetinam yetinam added this to the v0.3 milestone Jan 10, 2023
@miili
Copy link
Contributor Author

miili commented Jan 10, 2023

@yetinam, do you want to integrate https://github.com/codespell-project/codespell into pre-commit?

@yetinam
Copy link
Member

yetinam commented Jan 10, 2023

I think I'd skip codespell for now.

For the CI failures, they seem to be related to the version parsing. I'll look into it in more detail in the next days.

@yetinam
Copy link
Member

yetinam commented Jan 11, 2023

Hi! There seems to be an issue with the version inference from setuptools_scm. When installing the branch in a clean environment, the version number is 0.0.0. However, running setuptools_scm.get_version in the repo gives the correct version number. This issue is also causing the failing tests. Could you have a look into it?

With the new setup, editable installations are by default assigned version number 0.0.0. To still be able to load weights, the version 0.0.0 allows loading weights irrespective of their version requirements.
@yetinam
Copy link
Member

yetinam commented Jan 11, 2023

I fixed the CI failures but there is still an issue with the version inference. Try running python setup.py sdist bdist_wheel locally. It will build wheels with version number 0.0.0. In addition, it issues warnings that it does not find the subpackages of seisbench, e.g., seisbench.data. We'll need to fix this before merging.

@miili
Copy link
Contributor Author

miili commented Jan 11, 2023

version inference should work now, see 4e947ca

It will never report 0.0.0. Maybe remove handling of those exceptions in the .py logic. Revert c282923?

Required for setuptools_scm to work correctly
@miili
Copy link
Contributor Author

miili commented Jan 11, 2023

It now infers 0.1 because the main branch (which I forked) is not tagged.

@yetinam
Copy link
Member

yetinam commented Jan 11, 2023

I think the issue is that the tags were not pulled because of how the checkout action works by default. A few commits on main have tags. I added a mitigation for that but let's see what the CI says. You can try running the build command locally on your branch. It gives 0.2.2-dev... which is consistent with the last tag on main.

@miili
Copy link
Contributor Author

miili commented Jan 11, 2023

looks good!

@miili
Copy link
Contributor Author

miili commented Jan 11, 2023

Are all of the workarounds in c282923 necessary?

@yetinam
Copy link
Member

yetinam commented Jan 11, 2023

Not so much actually... Check https://github.com/seisbench/seisbench/actions/runs/3894641535/jobs/6648911568 . The inferred version is still 0.1.dev, which is wrong because main has a 0.2.2 tag somewhere.

@miili
Copy link
Contributor Author

miili commented Jan 11, 2023

@yetinam
Copy link
Member

yetinam commented Jan 11, 2023

Are all of the workarounds in c282923 necessary?

I think the parts in the test are good practice because that makes the test independent of the actual seisbench version. The part for checking version requirements is debatable. But I think it's in general rather practical if for dev-version the model weight requirements are not enforced.

@miili
Copy link
Contributor Author

miili commented Jan 11, 2023

The dev in string logic is obscure. You might run into problems between development/production usage.

@yetinam
Copy link
Member

yetinam commented Jan 11, 2023

https://github.com/seisbench/seisbench/actions/runs/3894650340/jobs/6648933093 passes now

Yes, but it infers seisbench-0.1.dev1+gc1f5b63. I'll cut the "dev" logic (because I admit it's a weird hack) but I'm pretty sure that will make the CI crash again.

@miili
Copy link
Contributor Author

miili commented Jan 11, 2023

It looks like main and the maintenance* branches have diverged a little bit. Maybe it would be straight forward to stick to main together with a development branch.

The need for "independent" maintenance branches (imo) is only needed for major versions.

From semantic versioning:

MAJOR version when you make incompatible API
MINOR version when you add functionality in a backwards compatible manner
PATCH version when you make backwards compatible bug fixes

@yetinam
Copy link
Member

yetinam commented Jan 11, 2023

Yep, fails as expected. There's apparently still some issue with pulling the tags because if you check in your branch history, there is a v0.2.1 tag on your branch and locally it works just fine. I'll have to leave for today but I can look at it again tomorrow. If you find a solution overnight, I wouldn't mind that either ;-)

@miili
Copy link
Contributor Author

miili commented Jan 11, 2023

@yetinam ping for workflow

@yetinam
Copy link
Member

yetinam commented Jan 11, 2023

Perfect, that did the job. Could you apply the same change to the build action too? Then I'll look over the PR tomorrow and can probably merge it.

Regarding the branching model, I'd decouple the discussion from this PR. Maybe you can open a separate issue about it and then we can have a discussion there?

@miili
Copy link
Contributor Author

miili commented Jan 11, 2023

The build process should work as is.

@yetinam
Copy link
Member

yetinam commented Jan 12, 2023

Just saw the error you fixed in the CI. Well, seems I was tired at that last commit. Thanks!

@yetinam yetinam merged commit 07434cb into seisbench:main Jan 12, 2023
@yetinam
Copy link
Member

yetinam commented Jan 12, 2023

Merged the PR now. Thanks a lot for this contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup: Use pyproject.toml Generate appropriate version number for main branch
2 participants