131 look into setting time limit on background validation task #159

jcadam14 · 2024-04-12T18:01:36Z

Closes #131

Added fastapil-utilities, which includes an async @retry_every decorator.
Added env vars to set the interval we check expired submissions, and the time diff from time now to check if something is considered expired
Added repo function to query over SUBMISSION_STARTED, SUBMISSION_UPLOADED, and VALIDATION_IN_PROGRESS and check the submission_time to time now, and set to VALIDATION_EXPIRED if greater
- I used submission_time, which I think is going to be moved under submitter, since the times between START, UPLOADED and IN_PROGRESS are relatively tiny. We don't capture or update a timestamp based on state transitions so I figured for this, it was 'good enough'.
Added new state to enum and alembic script for db update
Added pytests

…epo check

…nd-validation-task

github-actions · 2024-04-12T18:02:35Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
src/sbl_filing_api
config.py
src/sbl_filing_api/entities/models
model_enums.py
src/sbl_filing_api/entities/repos
submission_repo.py
src/sbl_filing_api/routers
filing.py
src/sbl_filing_api/services
submission_processor.py
Project Total

_{This report was generated by python-coverage-comment-action}

lchen-2101 · 2024-04-15T22:13:14Z

src/sbl_filing_api/main.py

+@repeat_every(seconds=settings.expired_submission_check_secs, wait_first=True, logger=log)
+async def check_expired_submissions() -> None:
+    await submission_repo.check_expired_submissions()


I think for MVP, this is fine; but in the grand scheme of things, this should probably be part of another non-api service that deals with collector type of tasks.
food for thought, another approach we might be able to do is with task timeout wrapper, something like

async with asyncio.timeout_at(asyncio.get_running_loop().time() + timeout_in_seconds): ...

Yeah this is definitely purely an MVP implementation to handle the 'just in case' situations where an exception or scenario we didn't think about creeps up and causes the frontend to continually spin because the latest submission never gets out of the in progress state.

I went with the single task polling the db route instead of a thread for each submission that expires that particular one ( assuming that is what you're alluding to there). I've done both in the past and had less headaches with a single periodic updater. I'm good to discuss more before squashing and merging. But yes, this is a temporary solution to an issue that more robust error handling recently implemented will probably take care of but gives the frontend warm fuzzies ;)

lchen-2101

LGTM for MVP, should revisit post-MVP

hkeeler

I was thinking this would be something more at the task level that would monitor how long the process has been running. I do like this approach, though. My only question is, how's this going to behave if we have multiple instances for the container running at the same time? They'll each be polling the database at the same frequency. Chances are they'll frequently try to each set it to VALIDATION_EXPIRED at the same time. Any potential issues with that?

I'm thinking back to a past life where we had many clients polling the same table, and sometimes updated it as well, and we'd get database deadlocks in certain scenarios, until we restructured the code.

In the long run, if we want/need to keep a process like this around, we may want to run it as a whole separate process...but not now if we don't have to. 😄

hkeeler · 2024-04-15T23:28:54Z

src/sbl_filing_api/entities/repos/submission_repo.py

+    stmt = select(SubmissionDAO).filter(SubmissionDAO.state.in_(check_states))
+    submissions = (await session.scalars(stmt)).all()
+    for s in submissions:
+        if abs(s.submission_time.timestamp() - datetime.now().timestamp()) > settings.expired_submission_diff_secs:


Do we need the abs? Can we just flip what's subtracted from what?

Suggested change

if abs(s.submission_time.timestamp() - datetime.now().timestamp()) > settings.expired_submission_diff_secs:

if datetime.now().timestamp() - s.submission_time.timestamp() > settings.expired_submission_diff_secs:

Also, can we add a WARN-level log statement in here when this happens?

Yup to both.

Also yeah definitely, wasn’t considering multiple pods. What I’ve done in the past is a single stateful set pod (you can singleton em) that handle this sort of thing. It would just be the scheduled task and would call an endpoint on the service (which could then be handled by any of the api pods)

There wasn’t any sort of timeout I could find to set on the background task itself. However talking with ye olde chat we could use a mixture of the background task, a wait_for task with timeout and gather them. The wait task timeout error could check, and then set, the sub state if it’s not in a good validation state. Problem with that approach is it would only handle situations where the sub actually entered into the validation flow. If someone failed outside of that (during S3 for example) that we for some reason don’t handle gracefully prior to the background task creation, we’d miss setting the expired state. Just a consideration to tying it to the background task

…nd-validation-task

jcadam14 · 2024-04-17T18:13:36Z

Still putting in the try/catch around S3 and all that, just wanted to get my start of the rewrite pushed

lchen-2101 · 2024-04-18T14:19:41Z

src/.env.local

+FS_DOWNLOAD_CONFIG__PROTOCOL="file"
+EXPIRED_SUBMISSION_CHECK_SECS=60


this one isn't needed anymore, right?

Nope thank you, removing

lchen-2101 · 2024-04-18T14:31:27Z

pyproject.toml

@@ -23,6 +23,7 @@ async-lru = "^2.0.4"
 fsspec = "^2024.2.0"
 s3fs = "^2024.2.0"
 httpx = "^0.26.0"
+fastapi-utilities = "^0.2.0"


sorry, one more, 😂

and probably need to update the lock file as well

Curses! removed

lchen-2101

LGTM

jcadam14 added 4 commits April 11, 2024 14:46

Added a @repeat_every check, env vars, VALIDATION_EXPIRED state and r…

ac50621

…epo check

Removed comment

913a9d0

Updated pytests

e3c9296

Merge branch 'main' into 131-look-into-setting-time-limit-on-backgrou…

30c7b6e

…nd-validation-task

jcadam14 requested review from hkeeler, lchen-2101 and nargis-sultani April 12, 2024 18:01

jcadam14 self-assigned this Apr 12, 2024

jcadam14 linked an issue Apr 12, 2024 that may be closed by this pull request

Look into setting time limit on background validation task #131

Closed

lchen-2101 reviewed Apr 15, 2024

View reviewed changes

lchen-2101 previously approved these changes Apr 15, 2024

View reviewed changes

hkeeler reviewed Apr 16, 2024

View reviewed changes

jcadam14 added 2 commits April 16, 2024 10:04

Flipped time delta check and added a logger.warn message

594f00c

Linting

dada53e

jcadam14 dismissed lchen-2101’s stale review via dada53e April 16, 2024 14:10

jcadam14 added 2 commits April 17, 2024 10:20

Merge branch 'main' into 131-look-into-setting-time-limit-on-backgrou…

89e25e6

…nd-validation-task

Rewrite using asyncio

666bf66

Added exception handling prior to the background_task

fadcebf

lchen-2101 reviewed Apr 18, 2024

View reviewed changes

Removed unneeded env var, updated default timeout sec to 60

e0a9745

lchen-2101 reviewed Apr 18, 2024

View reviewed changes

poetry removed fastapi-utlities

8335647

lchen-2101 approved these changes Apr 18, 2024

View reviewed changes

lchen-2101 merged commit 2b1884a into main Apr 18, 2024
3 checks passed

lchen-2101 deleted the 131-look-into-setting-time-limit-on-background-validation-task branch April 18, 2024 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

131 look into setting time limit on background validation task #159

131 look into setting time limit on background validation task #159

jcadam14 commented Apr 12, 2024 •

edited

Loading

github-actions bot commented Apr 12, 2024 •

edited

Loading

lchen-2101 Apr 15, 2024

jcadam14 Apr 15, 2024

lchen-2101 left a comment

hkeeler left a comment

hkeeler Apr 15, 2024

hkeeler Apr 16, 2024

jcadam14 Apr 16, 2024

jcadam14 commented Apr 17, 2024

lchen-2101 Apr 18, 2024

jcadam14 Apr 18, 2024

lchen-2101 Apr 18, 2024

lchen-2101 Apr 18, 2024

jcadam14 Apr 18, 2024

lchen-2101 left a comment

	if abs(s.submission_time.timestamp() - datetime.now().timestamp()) > settings.expired_submission_diff_secs:
	if datetime.now().timestamp() - s.submission_time.timestamp() > settings.expired_submission_diff_secs:

		FS_DOWNLOAD_CONFIG__PROTOCOL="file"
		EXPIRED_SUBMISSION_CHECK_SECS=60

131 look into setting time limit on background validation task #159

131 look into setting time limit on background validation task #159

Conversation

jcadam14 commented Apr 12, 2024 • edited Loading

github-actions bot commented Apr 12, 2024 • edited Loading

Coverage report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lchen-2101 left a comment

Choose a reason for hiding this comment

hkeeler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcadam14 commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lchen-2101 left a comment

Choose a reason for hiding this comment

jcadam14 commented Apr 12, 2024 •

edited

Loading

github-actions bot commented Apr 12, 2024 •

edited

Loading