Separate packages

[shyamd]

This deals with #255 and breaks up the various parts of the tools into induvidual packages. There is also an optimade meta-package that installs everything.

One thing we need to decide is where __version__ and __api_version__ go. I'm managing this break up via namespace packages which don't allow for an __init__.py at the namespace level. Right now they are both in the optimade-server, but I could understand them being moved into optimade-core.

[codecov]

Codecov Report

Merging #264 into master will decrease coverage by 90.03%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #264       +/-   ##
==========================================
- Coverage   90.03%   0.00%   -90.04%     
==========================================
  Files          54      47        -7     
  Lines        2268    1972      -296     
==========================================
- Hits         2042       0     -2042     
- Misses        226    1972     +1746     

Flag	Coverage Δ
#unittests	0.00% <0.00%> (-90.04%)	⬇️

Impacted Files	Coverage Δ
optimade-core/optimade/filterparser/__init__.py	0.00% <ø> (ø)
optimade-core/optimade/filterparser/lark_parser.py	0.00% <ø> (ø)
optimade-core/optimade/models/__init__.py	0.00% <ø> (ø)
optimade-core/optimade/models/baseinfo.py	0.00% <ø> (ø)
optimade-core/optimade/models/entries.py	0.00% <ø> (ø)
optimade-core/optimade/models/index_metadb.py	0.00% <ø> (ø)
optimade-core/optimade/models/jsonapi.py	0.00% <ø> (ø)
optimade-core/optimade/models/links.py	0.00% <ø> (ø)
optimade-core/optimade/models/optimade_json.py	0.00% <ø> (ø)
optimade-core/optimade/models/references.py	0.00% <ø> (ø)
... and 82 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c8c47f...7b9e0c8. Read the comment docs.

[shyamd]

I've got this pretty far, but I'm sorta stuck on checks. The eager deps workflow doesn't make much sense. They don't recreate the same tests as the main tests, and we have an arbitrary requirements text file buried in the github actions?

As for the API, i'm completely stumped on the pre-commit and OpenAPI actions. They both work on my computer.

The Docker GH Validator will just fail till we merge this in and update that.

[CasperWA]

Yeah, that didn't work. The issue is that the optimade-* packages are not on PyPi in the current form, so it is failing.
These lines are the problem:
https://github.com/Materials-Consortia/optimade-validator-action/blob/3be5ed987b56165e7182907f6418459d71b3adb3/entrypoint.sh#L10-L13

In our CI it always defaults to the last else statement here, since we're using the following in the CI:
https://github.com/Materials-Consortia/optimade-python-tools/blob/master/.github/workflows/deps_lint.yml#L114

[CasperWA]

Oh, it is:
These lines:

optimade-python-tools/.github/workflows/deps_lint.yml

Lines 108 to 114 in b5cc923

	- name: Test server, including OPTIONAL base URLs
	uses: Materials-Consortia/optimade-validator-action@v1
	with:
	port: 3213
	path: /
	all versioned paths: yes
	validator version: ${{ github.sha }} # This ensures the head of a PR or latest push commit is tested

Is where the failure is occurring.

Not a failure. It's what it's supposed to do: Use the validator from the PR commit.

[CasperWA]

Yeah, that didn't work. The issue is that the optimade-* packages are not on PyPi in the current form, so it is failing.
These lines are the problem:
https://github.com/Materials-Consortia/optimade-validator-action/blob/3be5ed987b56165e7182907f6418459d71b3adb3/entrypoint.sh#L10-L13

The validator isn't running via the validator action here though? This repo is only tested with the acutal validator code internally, so I would've thought the local install inside the docker image should work fine?

EDIT: phrasing

Actually, there is no need anymore for us to call the validator in our internal pytests, other than it's nice to have when running the tests locally. But for CI we're already running these tests for our "implementation" via the Docker CI job run.

[ml-evs]

Oh, it is:

ah, um, this is more convoluted than I remembered ><

[shyamd]

Ok, so it appears there is a dependency cycle that normally works because the structure of the repo doesn't change, but it's broken this one situation.

The GH-Validator makes an assumption on how the validator gets installed which is no longer the case for this one PR simply because there is no optimade-* packages on PyPI. Something has to have a broken merge. That's either going to be this repo or the GH-Validator. I'm more inclined to break this repo since the source of the break is the restructuring so it should be reflected here.

If you have an idea on how to do this without breaking a repo's CI tests, I'm all ears.

[CasperWA]

Ok, so it appears there is a dependency cycle that normally works because the structure of the repo doesn't change, but it's broken this one situation.

The GH-Validator makes an assumption on how the validator gets installed which is no longer the case for this one PR simply because there is no optimade-* packages on PyPI. Something has to have a broken merge. That's either going to be this repo or the GH-Validator. I'm more inclined to break this repo since the source of the break is the restructuring so it should be reflected here.

If you have an idea on how to do this without breaking a repo's CI tests, I'm all ears.

Right: It's expecting it to be pip installable from the root folder essentially. Hmm. I would probably.. Hmm.. Yeah, good question.
Maybe you should change the CI to point the validator to the master branch for now (and open an issue that this should eventually be changed back again), or even better, the latest current commit SHA in the master branch (then it will still work when merged). Since this PR is not changing the validator, but only the repository structure, it's fine.
Then we can release a v2 of the validator that uses the new structure (which is probably necessary, since the validator can not be made backwards-compatible as I see it). And finally revert back to the original way of running the CI in this repository.

[shyamd]

Yeah! This works for now.

[shyamd]

I forgot to update the release workflow.
What is check_released_tag.py for?

Also, each package will need a version. Do we just keep them all in sync with one version file? I'm thinking linked __version__.py might be a good idea? We could also make it so that the versions are independent.

[CasperWA]

I forgot to update the release workflow.
What is check_released_tag.py for?

It's trying to be a fail-safe of sorts to check the tag name (i.e., the version) matches the package __version__.

Also, each package will need a version. Do we just keep them all in sync with one version file? I'm thinking linked __version__.py might be a good idea? We could also make it so that the versions are independent.

Definitely just one single version number. Everything else gets too confusing, right?

[shyamd]

Any issues with using the git tag for __version__ then? __api_version__ still gets set manually, which I think is reasonable.

[CasperWA]

Any issues with using the git tag for __version__ then? __api_version__ still gets set manually, which I think is reasonable.

There is the discrepancy of the inital v between these, which is not desired in __version__, but otherwise fine I guess.
The only issue is that if one makes a mistake and writes the wrong version tag there is not much to catch it, while for the workflow we have now, there are two critical points where it may be caught: The PR that updates the version number & the CI that checks the subsequent tag against the version number.

Also, the "manual" part here is mitigated somewhat by the invoke tasks we have - but sure, they are still "manual".

[shyamd]

There is the discrepancy of the inital v between these, which is not desired in __version__, but otherwise fine I guess.

What do you mean by this? Like the tag will have v and the actual value in __version__ shouldn't have the v

[CasperWA]

There is the discrepancy of the inital v between these, which is not desired in __version__, but otherwise fine I guess.

What do you mean by this? Like the tag will have v and the actual value in __version__ shouldn't have the v

yepper

[shyamd]

Ah setuptools_scm takes care of that. You tag as v0.11.1 etc and it removes the v

[CasperWA]

Ah setuptools_scm takes care of that. You tag as v0.11.1 etc and it removes the v

Going through their readme, it seems a lot of trouble for a small issue.
I don't like the idea of depending on libs even before declaring actual dependencies.
While I agree that the current workflow is a bit... muddy.. dirty.. something like that, it is at least self-contained, with no need for extra deps (but instead a constant maintenance). But must importantly it set in place the aforementioned safe guards against common human errors.

Hmm.. I'm seeing a lot both for and against this. Would it be possible to address this in a separate PR maybe or would it fit best here if the change is needed?

[shyamd]

That's fine. I've got to figure out how to do one central file for versioning then.

[shyamd]

Going through their readme, it seems a lot of trouble for a small issue.
I don't like the idea of depending on libs even before declaring actual dependencies.

It's a huge issue. Just because we write 50 asserts to check versions doesn't mean they capture the important edge cases. setuptools_scm on the other hand is written and maintained by PyPa. Packages and versioning are what they deal with. I trust their code over ours any day.

I'm not seeing a good way to maintain multiple packages with consistent versions that's not a lot of untested infrastructure on our part. The alternative is to just go back to the giant mono-package.

[shyamd]

Versioning is turning out to be a nightmare. So I'm going to close this for now and build a PR for some of the fixes and updates that can be incorporated back into the mono-package structure.