Skip to content

Clean up delphi_utils (take 2) #2014

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

Merged
merged 24 commits into from
Sep 9, 2024
Merged

Conversation

melange396
Copy link
Contributor

Description

migrating setup.py to pyproject.toml and cleaning up dependencies

Changelog

Itemize code/test/documentation changes and files added/removed.

  • added pyproject.toml

Associated Issue(s)

(this is a redo of #1984, which was reverted in #2001 due to problems with Jenkins builds)

Copy link
Contributor

@dshemetov dshemetov left a comment

Choose a reason for hiding this comment

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

I didn't catch in the previous PR that the individual indicators don't actually need the dev version of the delphi utils, so made those suggestions. If the indicators have dependency issues because of these changes, we should add the dependencies to the indicator's dependency list (seems like good form to isolate dev dependencies).

@dshemetov dshemetov mentioned this pull request Aug 6, 2024
3 tasks
@aysim319 aysim319 force-pushed the 1973-clean-up-delphi-utils-take-2 branch from d99598e to b7f07c0 Compare August 7, 2024 14:12
@aysim319
Copy link
Contributor

aysim319 commented Aug 7, 2024

I didn't catch in the previous PR that the individual indicators don't actually need the dev version of the delphi utils, so made those suggestions. If the indicators have dependency issues because of these changes, we should add the dependencies to the indicator's dependency list (seems like good form to isolate dev dependencies).

most of these need for mock, the only other package that has mock as a requirement in setup.py is google symptoms. But google symptom linter acted weird when I took the dev from the makefile off. see comment

@aysim319 aysim319 force-pushed the 1973-clean-up-delphi-utils-take-2 branch from 5ba012d to 068d4d0 Compare August 7, 2024 14:21
@aysim319
Copy link
Contributor

aysim319 commented Aug 7, 2024

Do we also want to convert the rest of the indicator to pyproject.toml? maybe putting in the template_python to starting thing off?

@dshemetov
Copy link
Contributor

dshemetov commented Aug 7, 2024

most of these need for mock

Right, I'm saying we should add mock to each indicator's individual setup.py instead of grabbing it from _delphi_utils' dev dependencies. Sounds good on migrating the indicators to pyproject.toml, then each indicator can have its own "dev" set of packages and we can put mock in there.

Also, please add requests to the _delphi_utils pyproject.toml, it was in my suggestion above.

@aysim319
Copy link
Contributor

aysim319 commented Aug 7, 2024

Oh I didn't see it before 🤦‍♀️Thanks for pointing it out

@dshemetov
Copy link
Contributor

dshemetov commented Aug 7, 2024

FYI, this Jenkins script will need to be adjusted to check for the presence of pyproject.toml instead.

@aysim319
Copy link
Contributor

aysim319 commented Aug 8, 2024

most of these need for mock

Right, I'm saying we should add mock to each indicator's individual setup.py instead of grabbing it from _delphi_utils' dev dependencies. Sounds good on migrating the indicators to pyproject.toml, then each indicator can have its own "dev" set of packages and we can put mock in there.

Okay just making sure I'm understanding this properly; do we want to convert the existing indicators also to pyproject.toml with the same setup as delphi_utils that has an optional requirement in this PR or do we want to punt that for another PR? if we are punting to another PR, I feel like it makes more sense to leave it as is. Especially since this is
a step 2 of the bigger cleaning up delphi_utils (#1973)

@dshemetov
Copy link
Contributor

dshemetov commented Aug 8, 2024

I don't have a strong preference. Getting delphi_utils cleaned is the key task here and this PR is close to taking care of that (I still think we should remove the ".[dev]" from the individual indicators and debug this CI). Porting the indicators to pyproject.toml is nice to have for consistency, but not critical. If I had to pick, I'd say just move the indicators in another PR.

@dshemetov dshemetov force-pushed the 1973-clean-up-delphi-utils-take-2 branch 4 times, most recently from 4c63558 to ada86c2 Compare August 8, 2024 19:00
* pandas[excel]<2 doesn't work, go back to xlrd and comment
* add mock to changehc
* add requests
* remove setup.py from template
@dshemetov dshemetov force-pushed the 1973-clean-up-delphi-utils-take-2 branch from ada86c2 to 46ef13b Compare August 8, 2024 19:03
Copy link
Contributor

@dshemetov dshemetov 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 good to go. Let's make an issue to migrate the indicators to pyproject.toml and link to this thread. Let's remember to squash merge, so the commit log gets cleaned.

@aysim319 aysim319 merged commit 86399ad into main Sep 9, 2024
16 checks passed
minhkhul added a commit that referenced this pull request Sep 12, 2024
* doc: add logger examples to README and format

* deployment + staging details

* wrap lines

* Update _template_python/INDICATOR_DEV_GUIDE.md

Co-authored-by: David Weber <[email protected]>

* Update _template_python/INDICATOR_DEV_GUIDE.md

Co-authored-by: David Weber <[email protected]>

* Update _template_python/INDICATOR_DEV_GUIDE.md

Co-authored-by: David Weber <[email protected]>

* Update INDICATOR_DEV_GUIDE.md with mysql syntax

* Update INDICATOR_DEV_GUIDE.md reorder instruction on cron jobs

* switch from input_file to input_dir (#1995)

* adjust nssp max_age

* add pyproject.toml

* made changes based on suggestion

* update ci config

* separate new line

Co-authored-by: Dmitry Shemetov <[email protected]>

* fix: add explicit boto and moto dependencies to indicators

* trying to get package data to show

* installing dev package before testing in ci

* adding dependency

* move comment to right package

* Update _delphi_utils_python/pyproject.toml

Co-authored-by: Dmitry Shemetov <[email protected]>

* Revert "Clean up delphi_utils "

* refactoring date parsing to seperate file

* first implimentation

* small cleanup and adding missing data

* changed wording and some logic

* made test more explicit

* add missing file

* cleaning up tests

* consolidating tests into one

* fixed wording

* consolidating test/fixture

* clean up test

* Update google_symptoms/delphi_google_symptoms/patch.py

remove old code; not used anymore

Co-authored-by: nmdefries <[email protected]>

* clean up lint

* remove teardown

* cleaning and renaming

* more explicit branching for regular run vs patching, renaming, cleaning

* cleanup and rewrite logic

* avoid linking keywords in PR template

* Update _delphi_utils_python/README.md

Co-authored-by: george <[email protected]>

* Refactor Jenkins pipeline

- Remove usage of parallel in the build and package stages
- Retain usage of parallel in the deploy stages
- Separate build and package into separate scripts to support build-only stages
- Add a build-only stage for dev/feature branches

* Add separate build and package scripts

* Update script comments

* Remove unused script

* changes based on suggestion

* standardizing output dir in test

* changing back to session scope

* more logging and update params

* adding back conditional for num_export_days

* test_run does not like fixtures

* make deep copies of params fixtures

* linting

* test patch check if num_export_days is set or not

* patch fn takes params as an arg, like run_module

* list values added using actual day as an example

* move pull date creation into pull_gs_data

* don't modify num_export_days; define start/end date in terms of it and lag

* refer to params by name; patch_flag -> custom_run flag

* use test data based on recent actual indicator run

* say where gold test data came from

* not all date_utils tests need metadata

* formatting

* fn docs

* lint and cleanup

* adding more tests

* removed extra lag and made test more robust

* export_start_date is optional since the first available date is baked in

* need to account for historical / current run for adding lag

* adding validation script

* fix typo

* fixing typo in test

* lint

* sample run for validation

* remove unused input

Co-authored-by: nmdefries <[email protected]>

* remove unused import

* added progress chunk to limit logging (#2024)

* added progress chunk to limit logging

* lint

* comment based on suggestion

* add hhs geo level (#2036)

* Add discussion of patching functionality to indicator manual (#2031)

* basic patching info

* custom_run

* add hhs to geo section

* de-emphasize R in contributing doc, and add links

* ref/issue date intro

* naming and force push standards

* secret and params links

* Clean up delphi_utils (take 2) (#2014)

* add pyproject.toml

* made changes based on suggestion

* update ci config

* separate new line

Co-authored-by: Dmitry Shemetov <[email protected]>

* fix: add explicit boto and moto dependencies to indicators

* trying to get package data to show

* installing dev package before testing in ci

* adding dependency

* move comment to right package

* Update _delphi_utils_python/pyproject.toml

Co-authored-by: Dmitry Shemetov <[email protected]>

* more suggestion

* adding pyproject.toml to template

* Update changehc/Makefile

* Update claims_hosp/Makefile

* Update doctor_visits/Makefile

* Update google_symptoms/Makefile

* Update hhs_hosp/Makefile

* Update nchs_mortality/Makefile

* Update nssp/Makefile

* Update nwss_wastewater/Makefile

* Update quidel_covidtest/Makefile

* Update sir_complainsalot/Makefile

* fix: dependence bugs and cleanup
* pandas[excel]<2 doesn't work, go back to xlrd and comment
* add mock to changehc
* add requests
* remove setup.py from template

* build: fix Jenkins build script

---------

Co-authored-by: Amaris Sim <[email protected]>
Co-authored-by: aysim319 <[email protected]>
Co-authored-by: Dmitry Shemetov <[email protected]>

* fix: build-indicators.sh looks for setup.py and pyproject.toml (#2048)

* fix: build-indicators.sh looks for setup.py again

Reverting a premature change.

* fix: readability and future-proof

* fix: more readability

* chore: bump delphi_utils to 0.3.25

* chore: bump covidcast-indicators to 0.3.56

* [create-pull-request] automated change

---------

Co-authored-by: Dmitry Shemetov <[email protected]>
Co-authored-by: minhkhul <[email protected]>
Co-authored-by: Nat DeFries <[email protected]>
Co-authored-by: minhkhul <[email protected]>
Co-authored-by: David Weber <[email protected]>
Co-authored-by: george <[email protected]>
Co-authored-by: Amaris Sim <[email protected]>
Co-authored-by: aysim319 <[email protected]>
Co-authored-by: Brian Clark <[email protected]>
Co-authored-by: Delphi Deploy Bot <[email protected]>
Co-authored-by: minhkhul <[email protected]>
@nmdefries nmdefries deleted the 1973-clean-up-delphi-utils-take-2 branch December 10, 2024 22:23
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.

3 participants