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

refactor: bundle covidcast==0.2.2 in delphi_utils #1985

Closed
wants to merge 2 commits into from

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Jul 3, 2024

Description

Tried adding a simple wrapper around delphi_epidata.Epidata in here to avoid covidcast, but it grew to become most of the covidcast client from the other repo. So I just bundled the covidcast==0.2.2 in here, which removes transitive dependencies on plotting and geo libaries and unpins Pandas, but also doesn't require us to do any new testing of the client library.

Eventually, we should replace this with epidatpy, but that is possibly months from release.

The main thing we should pay attention to is the Pandas version changing our code behavior (both in client and in indicator pipelines) - we went from pandas-1.5.3 to pandas-2.0.3 (in CI below; not sure why it didn't go up to 2.2.x, pip might just prefer installing a cached version that satisfies the resolver). The unit tests pass below, but it's possible they missed some things. We'll need to look at what behaviors changed from 1.5.3 to 2.0.0. Or we can just pin to 1.5.3 and punt the Pandas bump to another PR.

Changelog

  • Moved covidcast.py from here to this repo and brought the test file over as well.
  • Updated covidcast imports in this repo.

Fixes

@dshemetov dshemetov requested a review from melange396 July 3, 2024 00:12
Copy link
Contributor

@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.

This is an effective stop-gap for removing the dependency implications of #1972 (and it also partially fulfills #1931 and #1987 ), but it doesnt address the covidcast client inefficiencies mentioned in #1931. It doesnt "replace" the covidcast client as much as it just sources it from a different place...

@dshemetov
Copy link
Contributor Author

Superseded by #2004

@dshemetov dshemetov closed this Sep 9, 2024
@dshemetov dshemetov deleted the ds/covidcast-port branch September 9, 2024 21:13
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