Skip to content

[WIP] API server code health pass #1058

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

[WIP] API server code health pass #1058

wants to merge 8 commits into from

Conversation

rzats
Copy link
Contributor

@rzats rzats commented Dec 19, 2022 •
edited
Loading

Closes #998.

Prerequisites:

  • Unless it is a documentation hotfix it should be merged against the dev branch
  • Branch is up-to-date with the branch to be merged with, i.e. dev
  • Build is successful
  • Code is cleaned up and formatted

Summary

Fixes a set of issues in the server code as seen here.

  • fix up _handle_lag_issues_as_of()
  • rework set_order()
  • remove get_related_signals()
  • remove _db.py, move configuration settings which are actually used elsewhere
  • rename "pair" (time, geo, sourcesignal) classes
  • move parse & extract to proper location
  • remove global flask.request references
  • reword methods like parse & extract to match their actual purpose
  • integrate a number of class definitions

@rzats rzats force-pushed the rzatserkovnyi/server-health branch 2 times, most recently from 6cb6e81 to 28bc63e Compare December 19, 2022 13:45
@rzats rzats force-pushed the rzatserkovnyi/server-health branch from 28bc63e to 22d505d Compare December 19, 2022 13:50
@rzats rzats force-pushed the rzatserkovnyi/server-health branch 3 times, most recently from 753a913 to d0c7da0 Compare December 19, 2022 15:19
@rzats rzats force-pushed the rzatserkovnyi/server-health branch from d0c7da0 to 9451599 Compare December 19, 2022 15:27
melange396
melange396 requested changes Dec 22, 2022
View reviewed changes
Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of letting this snowball into one huge megalithic PR (encompassing many different types of changes) that auto-closes the referenced issue, it might be better to create smaller targeted PRs (that are more easily digested individually) and then ultimately close the PR by hand. It'll simplify the overall review process by separating concerns.

I don't think "XyzFilter" (TimeFilter, GeoFilter, etc) is a great option for renaming... Its not really a "filter" until it is applied, and there isnt anything about these objects (that specify sets of times/locations/signalnames) that is intrinsically filter-like. How about "TimeSpec" instead (or i could even be convinced to use "TimeSet")? I apologize, i should have been more explicit in my issue comment on the "TimePair" name being bad, and suggested these options then.

src/acquisition/covidcast/covidcast_meta_cache_updater.py
@@ -15,7 +15,7 @@ def get_argument_parser():

parser = argparse.ArgumentParser()
parser.add_argument("--log_file", help="filename for log output")
parser.add_argument("--num_threads", type=int, help="number of worker threads to spawn for processing source/signal pairs")
parser.add_argument("--num_threads", type=int, help="number of worker threads to spawn for processing source/signal filters")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
parser.add_argument("--num_threads", type=int, help="number of worker threads to spawn for processing source/signal filters")
parser.add_argument("--num_threads", type=int, help="number of worker threads to spawn for processing source/signal pairs")

this was acceptable usage, each unique pair of (source,signal) makes up a workload

src/acquisition/covidcast/database.py
@@ -551,7 +551,7 @@ def worker():
try:
while True:
(source, signal) = srcsigs.get_nowait() # this will throw the Empty caught below
logger.info("starting pair", thread=name, pair=f"({source}, {signal})")
logger.info("starting filter", thread=name, filter=f"({source}, {signal})")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.info("starting filter", thread=name, filter=f"({source}, {signal})")
logger.info("starting pair", thread=name, pair=f"({source}, {signal})")

this was acceptable usage, each unique pair of (source,signal) makes up a workload

src/server/_params.py
return [v for vs in s for v in vs.split(",")]


IntRange = Union[Tuple[int, int], int]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is only referenced in one place... we should either remove it or expand its usage.

src/server/endpoints/covidcast.py
Comment on lines +112 to +117
if issues:
_apply_issues_filter(q, issues)
elif lag is not None:
_apply_lag_filter(q, lag)
elif as_of is not None:
_apply_as_of_filter(q, as_of)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems silly to check if field / if field is not None here when each of these methods do the same test internally. it also seems silly to silently leave out a filtering condition for lag when issue is also specified, or to leave out as_of when lag or issue is specified... i claim that either (1) these 3 conditions should all be applied to the resulting SQL if they are specified in any combination, or (2) an error should be produced if they are truly intended to be mutually exclusive. i think it is more correct to do the former; that is, leave out the if/elif/elif lines and just call the 3 methods here. thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applying all three makes sense if it won't break any existing workflows!

@rzats
Copy link
Contributor Author

rzats commented Dec 22, 2022

@melange396 thanks for the comments! I'll close this off for now and split it into a few separate PRs.

@rzats rzats closed this Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melange396 melange396 melange396 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

API server code health improvements
2 participants
@rzats @melange396