Initial work for v1 #268

jkeifer · 2024-05-02T23:51:23Z

This PR is all a starting point and we'll need to do more, but it includes:

clean up all code no longer required with the new operational paradigm
- cirrus.cli code combed over for what is relevant to the cirrus management cli and moved there, the rest removed
- cirrus.core code removed
- cirrus.test module removed
reorganize remaining code
- cirrus.lib2 becomes cirrus.lib
- cirrus.builtins becomes cirrus.lambda_handlers and flattened
switch to pyproject.toml and pip-compile for deps
- though I am uncertain how we should manage versions given that we do want to be able to install this package into a virtual env with pip from pypi (i.e., requirements files alone ain't going to cut it)
pull in cirrus-mgmt cli code and install as cirrus console script
- code from Make CLI a standalone tool with cloud-managed configs cirrus-mgmt#10 where I made the cli standalone and implemented a PoC for how to manage config from AWS resources
docs dependency on cirrus-docs removed in favor of plain sphinx build and docs sources moved into docs/
I also looked for additional dead code that wasn't necessarily dead just from this work and removed it.
I pulled out all the backwards-compatibility code for deprecated features that I could find:
- output_options support
- support for the FAILED and INVALID SNS topics
- removed InvalidInput exception in this cirrus.lib
- legacy API support
I had concerns about recent changes to BatchHandler and the subclasses that were created, SNSPublisher and SQSPublisher. I took this opportunity to address those concerns.

jkeifer · 2024-05-02T23:54:23Z

Tests currently fail because

CI needs to be updated
The management cli tests have not been updated per the changes to that component

I do not know if I will be able to finish the cli updates for a new config format nor fix the tests. One potential blocker is not having a new deployment to test against, which means if I do work on this I don't know if I'll derive the correct config file format. But I'll see if I can get that in.

jkeifer · 2024-05-03T22:27:15Z

I want to question if we should also remove support for non-array process values, as we did in swoop... This seems like a great opportunity to do it.

codecov · 2024-05-03T23:18:29Z

Codecov Report

Attention: Patch coverage is 66.09195% with 295 lines in your changes are missing coverage. Please review.

Project coverage is 77.30%. Comparing base (8f82df5) to head (ac1d243).

Files	Patch %	Lines
src/cirrus/management/deployment.py	33.91%	113 Missing ⚠️
src/cirrus/management/commands/manage.py	65.03%	50 Missing ⚠️
src/cirrus/management/deployment_pointer.py	57.14%	30 Missing ⚠️
src/cirrus/management/utils/click.py	42.10%	22 Missing ⚠️
src/cirrus/lib/eventdb.py	81.15%	13 Missing ⚠️
src/cirrus/management/commands/payload.py	64.00%	9 Missing ⚠️
src/cirrus/lib/process_payload.py	79.48%	8 Missing ⚠️
src/cirrus/lambda_functions/api.py	75.86%	7 Missing ⚠️
src/cirrus/lib/statedb.py	84.78%	7 Missing ⚠️
src/cirrus/management/utils/templating.py	0.00%	7 Missing ⚠️
... and 10 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #268      +/-   ##
==========================================
- Coverage   82.95%   77.30%   -5.65%     
==========================================
  Files          57       26      -31     
  Lines        3033     1855    -1178     
==========================================
- Hits         2516     1434    -1082     
+ Misses        517      421      -96

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkeifer · 2024-05-03T23:23:12Z

Tests are fixed. I am explicitly ignoring failures on the management tests right now.

ircwaves · 2024-05-06T13:11:51Z

@jkeifer says:

I want to question if we should also remove support for non-array process values, as we did in swoop... This seems like a great opportunity to do it.

Not privy to the motivations in swoop, but that is a pattern which I have seen some developers confused by, returning a list that is usually empty or 1 member. I was implored to change it to a -> Item | None, with some rational that it was more idiomatic. That said, I was never that upset by for result in process(): in my code. So, some sort of note that emphasizes that use-cases for list-of-results might be warranted.

philvarner · 2024-05-06T19:36:25Z

Maybe lambda_handlers should be named lambda_functions?

philvarner · 2024-05-06T19:38:08Z

wrt to deps, I think it's reasonable to have exact pinned deps, since this is the key dependency for the projects that use it

philvarner · 2024-05-06T19:38:31Z

why "requirements files alone ain't going to cut it"?

jkeifer · 2024-05-06T19:42:55Z

@jkeifer says:

I want to question if we should also remove support for non-array process values, as we did in swoop... This seems like a great opportunity to do it.

Not privy to the motivations in swoop, but that is a pattern which I have seen some developers confused by, returning a list that is usually empty or 1 member. I was implored to change it to a -> Item | None, with some rational that it was more idiomatic. That said, I was never that upset by for result in process(): in my code. So, some sort of note that emphasizes that use-cases for list-of-results might be warranted.

@ircwaves I'm confused by your response and the mention of items/results. I am speaking about the process payload format, which supports an array of process definitions for workflow chaining. That is, the payload format is, roughly,

{
    "type": "FeatureCollection",
    "features": [Items],
    "process": ProcessDef | [ProcessDef],
}

What I'm proposing here is that we make process just [ProcessDef] rather than supporting the single process definition case without an array. As to why we went with that in swoop, it was to simplify the payload format by enforcing the consistency, which also reduces the test cases we have to be concerned about.

jkeifer · 2024-05-06T19:44:58Z

@philvarner

why "requirements files alone ain't going to cut it"?

Unless you know something I don't, to enable this to package to be pip-installable with the dependency pinning described in requirements.txt we have to have that pinning in pyproject.toml to have it be part of the package metadata.

philvarner · 2024-05-06T19:46:45Z

@philvarner

why "requirements files alone ain't going to cut it"?

Unless you know something I don't, to enable this to package to be pip-installable with the dependency pinning described in requirements.txt we have to have that pinning in pyproject.toml to have it be part of the package metadata.

Oh, I see what you're saying now.

src/cirrus/lambda_handlers/api.py

philvarner · 2024-05-06T19:59:22Z

src/cirrus/lambda_handlers/api.py

+        if not result:
+            continue
+        if not combined_results:
+            combined_results = result


should this break? If not, I think it would be better if there was a check above to see if combined_results is set and rs is non-empty and then pop the last one.

I don't know, I did what I thought would preserve the existing behavior as best as I could reason about while also supporting the case where we have Nones passed in to satisfy type checking. Maybe the whole thing deserves a bit more consideration. It might be better to throw an exception rather than returning None so we can avoid this case altogether.

src/cirrus/lambda_handlers/post_batch.py

src/cirrus/lib/process_payload.py

ircwaves · 2024-05-06T22:00:19Z

confused by your response and the mention of items/results. I am speaking about the process payload format, which supports an array of process definitions for workflow chaining.

Ah, I see, I was thinking of the process function in a task. Process entry in a payload being an array of ProcessDefinitions makes sense.

See cirrus-geo/cirrus-mgmt#10 for the PR against cirrus-mgmt that this came from.

* Don't strip cirrus.lib.errors from exception type anymore * Remove InvalidInput -- only way people should get it is from old lib * More tests for Invalid

jkeifer · 2024-05-07T17:50:20Z

I rebased the changes on top of main to resolve the merge conflicts. I also renamed the lambda_handlers package to lambda_functions as suggested, and removed support for non-array process values.

Pyproject.toml supports loading metadata values from files. We can leverage this to use pip-compile with .in files to generate requirements files that also populate the package metadata. This gives us a way pin all our dependencies (direct and transitive) strictly even when installing from a remote package with no direct access to the requirements files.

jkeifer · 2024-05-07T19:51:08Z

I talked with @gadomski and he pointed me to a solution for the dependency concern I raised. In short, we can use dynamic metadata to load dependency lists from requirements files. This means we specify the direct dependencies in *.in files and the pip-compile workflow will generate requirements files that end up populating the dependency lists in the package metadata.

I also took a super quick pass through the README and remove all obviously out-of-date content. It will need further revisions but at least isn't completely erroneous.

Reverts the changes to the API functions to handle null responses from eventdb queries (which were added to appease type checking), with some further simplifications. The _query method on EventDB will now raise an exception if called when events are disabled. This is explicitly handled in the API and ignored.

jkeifer · 2024-05-08T21:15:45Z

@philvarner I made the changes we discussed to the API/EventDB stuff. See the most recent commit for the specific changes.

jkeifer marked this pull request as draft May 2, 2024 23:55

jkeifer force-pushed the jak/cleanup branch from 83b7d38 to a92475b Compare May 3, 2024 04:26

jkeifer mentioned this pull request May 3, 2024

Fix overly-broad exception expectations in tests #271

Open

jkeifer force-pushed the jak/cleanup branch from cd61647 to 2d62a37 Compare May 3, 2024 23:16

jkeifer marked this pull request as ready for review May 4, 2024 00:58

philvarner reviewed May 6, 2024

View reviewed changes

src/cirrus/lambda_handlers/api.py Outdated Show resolved Hide resolved

philvarner reviewed May 6, 2024

View reviewed changes

src/cirrus/lambda_handlers/post_batch.py Outdated Show resolved Hide resolved

philvarner reviewed May 6, 2024

View reviewed changes

src/cirrus/lib/process_payload.py Outdated Show resolved Hide resolved

jkeifer added 9 commits May 7, 2024 09:40

initial cleanup for TNG

4321891

add region as opt param to get_client/get_resource

0716711

remove submit_batch_job

147fa52

switch to pyproject.toml, compile deps, and update precommit

137d04b

move away from namespace package

935e11d

pull in management cli

209adda

See cirrus-geo/cirrus-mgmt#10 for the PR against cirrus-mgmt that this came from.

some linting/formatting fixes

90369b4

remove cirrus-docs dependency

16ebf1e

better gitignore

b262ed5

jkeifer added 14 commits May 7, 2024 09:43

more type/formatting/linting fixes

17579b3

linting/formatting/typing fixes to get pre-commit passing on all files

4af156f

remove legacy API support

a30db7d

refine InvalidInput exception support

4ebc095

* Don't strip cirrus.lib.errors from exception type anymore * Remove InvalidInput -- only way people should get it is from old lib * More tests for Invalid

remove deprecated FAILED and INVALID topic notifications

682a88f

remove some dead code

c109c56

remove some bad mgmt tests and ignore others

93f3c6d

get ci working again

8fecd13

get docs workflow working again

cc19b44

CONTRIBUTING.md updates

bb7183d

rework BatchHandler and subclasses

374958f

rename lambda_handlers to lambda_functions per PR feedback

7f9bb7f

remove redundant None from a dict get call

772499b

remove support for non-array process values in payload

6e5dda5

jkeifer force-pushed the jak/cleanup branch from 5c53643 to 6e5dda5 Compare May 7, 2024 17:31

jkeifer added 2 commits May 7, 2024 12:44

remove all obviously out-of-date info from README

8bc9072

philvarner approved these changes May 8, 2024

View reviewed changes

jkeifer merged commit f905104 into main May 8, 2024
2 of 4 checks passed

jkeifer deleted the jak/cleanup branch May 8, 2024 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial work for v1 #268

Initial work for v1 #268

jkeifer commented May 2, 2024 •

edited

jkeifer commented May 2, 2024

jkeifer commented May 3, 2024

codecov bot commented May 3, 2024 •

edited

jkeifer commented May 3, 2024

ircwaves commented May 6, 2024

philvarner commented May 6, 2024

philvarner commented May 6, 2024

philvarner commented May 6, 2024

jkeifer commented May 6, 2024

jkeifer commented May 6, 2024 •

edited

philvarner commented May 6, 2024

philvarner May 6, 2024

jkeifer May 6, 2024

ircwaves commented May 6, 2024

jkeifer commented May 7, 2024

jkeifer commented May 7, 2024

jkeifer commented May 8, 2024

Initial work for v1 #268

Initial work for v1 #268

Conversation

jkeifer commented May 2, 2024 • edited

jkeifer commented May 2, 2024

jkeifer commented May 3, 2024

codecov bot commented May 3, 2024 • edited

Codecov Report

jkeifer commented May 3, 2024

ircwaves commented May 6, 2024

philvarner commented May 6, 2024

philvarner commented May 6, 2024

philvarner commented May 6, 2024

jkeifer commented May 6, 2024

jkeifer commented May 6, 2024 • edited

philvarner commented May 6, 2024

philvarner May 6, 2024

Choose a reason for hiding this comment

jkeifer May 6, 2024

Choose a reason for hiding this comment

ircwaves commented May 6, 2024

jkeifer commented May 7, 2024

jkeifer commented May 7, 2024

jkeifer commented May 8, 2024

jkeifer commented May 2, 2024 •

edited

codecov bot commented May 3, 2024 •

edited

jkeifer commented May 6, 2024 •

edited