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

Modernize giftless #125

Merged
merged 20 commits into from
Dec 27, 2023
Merged

Modernize giftless #125

merged 20 commits into from
Dec 27, 2023

Conversation

athornton
Copy link
Collaborator

Build with 3.10 as a base and move all dependencies to their current versions.

@athornton
Copy link
Collaborator Author

So, this is a pretty good start, I think, at a modernized giftless. It now requires Python 3.10 or greater to run, but all its dependencies are up to date.

The major thing that needs attention, and I think it probably needs attention from the Datopian side, is that moving to a newer set of Google libraries broke the Google Storage testing suite, which is extremely opaque (being pytest-vcr cassettes recorded against some Datopian set of runs against their repos). Hopefully someone at Datopian knows how these were produced and can just recreate them using the new code as a base.

I'm confident that Google still works as a storage backend, although I have only tested it with Workload Identity credentials (since that's what I use) rather than stored keys. But it's working fine for my Git-LFS implementation.

The other thing is that I took out pytest-flake8, so right now there's no test that the codebase passes flake8 (I ran it by hand to make sure it still does, and I feel like this/isort/tox all ought to be pre-commit hooks too). pytest-flake8 is unmaintained and doesn't work with recent flake8s, and we should probably switch over to using ruff or black rather than flake8. That's work I didn't want to tackle right now. I've got some pre-commit hooks for other Rubin Observatory projects that can probably be repurposed here pretty easily to get us back up to snuff, and add ruff/black to the set of checks as well.

@rufuspollock
Copy link
Member

rufuspollock commented Dec 25, 2023

@athornton is this good to merge as is or do we need to wait on the other points you mention?

The major thing that needs attention, and I think it probably needs attention from the Datopian side, is that moving to a newer set of Google libraries broke the Google Storage testing suite, which is extremely opaque (being pytest-vcr cassettes recorded against some Datopian set of runs against their repos). Hopefully someone at Datopian knows how these were produced and can just recreate them using the new code as a base.

I'm wondering if should try redoing this testsuite in some more standardized way. (plus the original author of those tests is no longer with us!)

The other thing is that I took out pytest-flake8, so right now there's no test that the codebase passes flake8 (I ran it by hand to make sure it still does, and I feel like this/isort/tox all ought to be pre-commit hooks too). pytest-flake8 is unmaintained and doesn't work with recent flake8s, and we should probably switch over to using ruff or black rather than flake8. That's work I didn't want to tackle right now. I've got some pre-commit hooks for other Rubin Observatory projects that can probably be repurposed here pretty easily to get us back up to snuff, and add ruff/black to the set of checks as well.

Let's replace flake8 with black. Would you be up for that?


And thank-you again for the excellent well-explained contributions 🙏🚀

@athornton
Copy link
Collaborator Author

I think merging it as-is is better than not merging it, in that it's getting increasingly hard to install Python 3.7 since it's past EOL, and therefore it's been removed from brew and stuff like that. And the test suite...well...

I very very much agree that a test suite that mocks out the cloud providers' responses and doesn't rely on the opaque wire-traffic captures that are basically what pytest-vcr does, would be a lot more maintainable and usable going forwards...but that's going to be quite a bit of work. It's certainly worth doing, and there may already be useful testing libraries for AWS and GCP--we're not the first people to write services against them, after all. I don't know HOW much work, though, which worries me.

When I'm back at work (next week, assuming I'm not firefighting) I'll freshen up the github actions and tox with basically-what-we-use-in-the-Rubin-project for linting and style-guide formatting. It's not very different than what the flake8 here did, although we tend to use shorter lines....but black will handle that for us. That's going to be one very large and very boring commit, so I'll make sure that "apply new formatting standards" is its own commit.

@rufuspollock
Copy link
Member

@athornton great. Ok i'll merge this for now.

Also LMK if we should move (some parts) of this discussion to a general issue about "upgrading giftless" so it doesn't get lost across different PRs 🙂

@rufuspollock rufuspollock merged commit 90a1175 into datopian:main Dec 27, 2023
3 checks passed
@athornton
Copy link
Collaborator Author

athornton commented Dec 28, 2023

A general-purpose issue seems like a good idea to me. I'll list the couple of things that immediately come to mind:

  1. maintainable test suite
  2. build/push docker artifacts via GHA, with matching tags in GH and Docker Hub
    2a) Or, just (or additionally) push to ghcr.io, which simplifies authentication somewhat
  3. generally spiffing up pre-commit hooks and GHA

...when I'm back at my work computer next week I'll see if more occurs.

@rufuspollock
Copy link
Member

Ok created #126

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