Skip to content

make a "src/common/" directory (package?) to share between server and acquisition code #1037

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

Closed
melange396 opened this issue Nov 29, 2022 · 6 comments · Fixed by #1104
Closed
Assignees
Labels
code health readability, maintainability, best practices, etc help wanted refactor Substantial projects to make the code do the same thing, better.

Comments

@melange396
Copy link
Collaborator

to promote modularity and reduce code repetition, we should create a "./src/common/" directory (and potentially package too). this will require some changes to our docker definitions and maybe also other parts of our build process.

the first file/module to include here could be our structured logger code... see: #1027 (comment)_

@krivard
Copy link
Contributor

krivard commented Nov 29, 2022

Alternative: pull logger.py out of delphi_utils & into its own package, and include that in all three places (indicators, acquisition, server)

@melange396
Copy link
Collaborator Author

following up, these two files are now essentially identical:
./src/acquisition/covidcast/logger.py
./src/server/utils/logger.py

@melange396 melange396 added refactor Substantial projects to make the code do the same thing, better. code health readability, maintainability, best practices, etc labels Nov 29, 2022
@krivard
Copy link
Contributor

krivard commented Nov 29, 2022

@melange396
Copy link
Collaborator Author

a big part of the problem is that the server code cant "see" the acqusisition code but the acq can see the server.

the root of this lies in two different docker setups:

  • these files preserve the module structure for the delphi_web_python docker image: all of the code tree is visible and happy.
  • this definition for the delphi_web_epidata image keeps only src/server/ code: the non-server code gets chopped out completely.

fixing the server docker image might (almost) be as simple as something like this...
change this line to something like:

  RUN mkdir -p /app/delphi/epidata
  COPY ./src/* /app/delphi/epidata/

or perhaps just:

  RUN mkdir -p /app/delphi/epidata
  COPY ./src/server /app/delphi/epidata/
  COPY ./src/common /app/delphi/epidata/

then use environment variables $MODULE_NAME + $VARIABLE_NAME (or just $APP_MODULE) to run delphi.epidata.server.main:app as described here. (those env variables would need to be added to our dev Makefile, the github ci.yaml, and probably somewhere in our scheduler/automation system).

a nice bonus is that i think this will also let us turn all of the relative local imports in the server code into fully qualified ones.

@melange396
Copy link
Collaborator Author

in addition to the logging module[s] mentioned above, the newly extracted CovidcastRow (see #1044) is another great first candidates to go in the common/ tree. after that, we could pull stuff from src/server/utils/* , and probbly some of src/server/endpoints/covidcast_utils/*

@melange396
Copy link
Collaborator Author

this will probably also need to include some changes to deploy.json, similar to those recently seen in #1097

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health readability, maintainability, best practices, etc help wanted refactor Substantial projects to make the code do the same thing, better.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants