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

Config usage improvements #2729

Merged
merged 7 commits into from Mar 11, 2024
Merged

Config usage improvements #2729

merged 7 commits into from Mar 11, 2024

Conversation

ABrain7710
Copy link
Contributor

@ABrain7710 ABrain7710 commented Mar 9, 2024

Description

  • Start reducing usage of AugurConfig class and start using the database library methods so that the database access logic is centralized
  • Defined get_value and get_section in the database lib.py file
  • Define temporary_database_engine context method for use in cases where a process needs a short-lived engine like in the celery app setup where a database engine cannot be defined when the worker processes are forked from the parent

Signed commits

  • Yes, I signed my commits.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

pylint

augur/application/db/lib.py|1| C0114: Missing module docstring (missing-module-docstring)
augur/application/db/lib.py|10 col 39| W0621: Redefining name 'logger' from outer scope (line 8) (redefined-outer-name)
augur/application/db/lib.py|15 col 4| R1705: Unnecessary "elif" after "return", remove the leading "el" from "elif" (no-else-return)
augur/tasks/data_analysis/insight_worker/tasks.py|53 col 16| C0209: Formatting a regular string which could be an f-string (consider-using-f-string)
augur/tasks/data_analysis/insight_worker/tasks.py|49 col 4| W0612: Unused variable 'confidence' (unused-variable)
augur/tasks/git/dependency_libyear_tasks/core.py|1| C0114: Missing module docstring (missing-module-docstring)
augur/tasks/git/dependency_libyear_tasks/core.py|2| W0401: Wildcard import augur.application.db.models (wildcard-import)
augur/tasks/git/scc_value_tasks/tasks.py|1| C0114: Missing module docstring (missing-module-docstring)
augur/tasks/git/scc_value_tasks/tasks.py|3| W0401: Wildcard import augur.tasks.git.scc_value_tasks.core (wildcard-import)
augur/tasks/github/util/github_api_key_handler.py|11| C0412: Imports from package sqlalchemy are not grouped (ungrouped-imports)
augur/tasks/github/util/github_random_key_auth.py|7| R0903: Too few public methods (1/2) (too-few-public-methods)
augur/tasks/gitlab/gitlab_api_key_handler.py|15| C0412: Imports from package sqlalchemy are not grouped (ungrouped-imports)
augur/tasks/gitlab/gitlab_random_key_auth.py|9| R0903: Too few public methods (1/2) (too-few-public-methods)
augur/tasks/init/celery_app.py|205 col 4| C0415: Import outside toplevel (augur.tasks.git.facade_tasks.clone_repos) (import-outside-toplevel)
augur/tasks/init/celery_app.py|206 col 4| C0415: Import outside toplevel (augur.tasks.db.refresh_materialized_views.refresh_materialized_views) (import-outside-toplevel)
augur/tasks/init/celery_app.py|207 col 4| C0415: Import outside toplevel (augur.tasks.data_analysis.contributor_breadth_worker.contributor_breadth_worker.contributor_breadth_model) (import-outside-toplevel)
augur/tasks/init/celery_app.py|208 col 4| C0415: Import outside toplevel (augur.application.db.temporary_database_engine) (import-outside-toplevel)
augur/tasks/init/celery_app.py|239| W0613: Unused argument 'args' (unused-argument)
augur/tasks/init/celery_app.py|239| W0613: Unused argument 'kwargs' (unused-argument)

bind = '%s:%s' % (augur_config.get_value("Server", "host"), augur_config.get_value("Server", "port"))
timeout = int(augur_config.get_value('Server', 'timeout'))
workers = int(get_value('Server', 'workers'))
bind = '%s:%s' % (get_value("Server", "host"), get_value("Server", "port"))
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
C0209: Formatting a regular string which could be an f-string (consider-using-f-string)


else:
workers = int(get_value('Server', 'workers'))
bind = '%s:%s' % (get_value("Server", "host"), get_value("Server", "port"))
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
C0209: Formatting a regular string which could be an f-string (consider-using-f-string)

else:
workers = int(get_value('Server', 'workers'))
bind = '%s:%s' % (get_value("Server", "host"), get_value("Server", "port"))
timeout = int(get_value('Server', 'timeout'))


def worker_exit(server, worker):
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0613: Unused argument 'server' (unused-argument)

else:
workers = int(get_value('Server', 'workers'))
bind = '%s:%s' % (get_value("Server", "host"), get_value("Server", "port"))
timeout = int(get_value('Server', 'timeout'))


def worker_exit(server, worker):
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0613: Unused argument 'worker' (unused-argument)

@@ -311,14 +311,14 @@ def create_cache_manager() -> CacheManager:

return cache

def get_server_cache(config, cache_manager) -> Cache:
def get_server_cache(cache_manager) -> Cache:
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0621: Redefining name 'cache_manager' from outer scope (line 731) (redefined-outer-name)

log_level = get_value("Logging", "log_level")
celery_beat_process = None
celery_command = f"celery -A augur.tasks.init.celery_app.celery_app beat -l {log_level.lower()}"
celery_beat_process = subprocess.Popen(celery_command.split(" "))
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
R1732: Consider using 'with' for resource-allocating operations (consider-using-with)

@@ -256,11 +251,10 @@ def augur_stop(signal, logger, engine):
def cleanup_after_collection_halt(logger, engine):
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0621: Redefining name 'logger' from outer scope (line 28) (redefined-outer-name)

@@ -256,11 +251,10 @@ def augur_stop(signal, logger, engine):
def cleanup_after_collection_halt(logger, engine):
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0613: Unused argument 'engine' (unused-argument)


url = get_database_string()
engine = create_database_engine(url=url, poolclass=StaticPool)
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0621: Redefining name 'engine' from outer scope (line 7) (redefined-outer-name)

with get_session() as session:


# TODO temporary until added to the DB schema
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0511: TODO temporary until added to the DB schema (fixme)


url = get_database_string()
temporary_database_engine = create_database_engine(url=url, poolclass=StaticPool)
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0621: Redefining name 'temporary_database_engine' from outer scope (line 42) (redefined-outer-name)

@@ -0,0 +1,97 @@
import sqlalchemy as s
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
C0114: Missing module docstring (missing-module-docstring)


logger = logging.getLogger("db_lib")

def convert_type_of_value(config_dict, logger=None):
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0621: Redefining name 'logger' from outer scope (line 8) (redefined-outer-name)


data_type = config_dict["type"]

if data_type == "str" or data_type is None:
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
R1705: Unnecessary "elif" after "return", remove the leading "el" from "elif" (no-else-return)

contamination = get_value('Insight_Task', 'contamination')
confidence = get_value('Insight_Task', 'confidence_interval') / 100
api_host = get_value('Server', 'host')
api_port = get_value('Server', 'port')

logger.info("Discovering insights for repo {}\n".format(repo_git))
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
C0209: Formatting a regular string which could be an f-string (consider-using-f-string)

@@ -205,8 +205,10 @@ def setup_periodic_tasks(sender, **kwargs):
from augur.tasks.git.facade_tasks import clone_repos
from augur.tasks.db.refresh_materialized_views import refresh_materialized_views
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
C0415: Import outside toplevel (augur.tasks.db.refresh_materialized_views.refresh_materialized_views) (import-outside-toplevel)

@@ -205,8 +205,10 @@ def setup_periodic_tasks(sender, **kwargs):
from augur.tasks.git.facade_tasks import clone_repos
from augur.tasks.db.refresh_materialized_views import refresh_materialized_views
from augur.tasks.data_analysis.contributor_breadth_worker.contributor_breadth_worker import contributor_breadth_model
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
C0415: Import outside toplevel (augur.tasks.data_analysis.contributor_breadth_worker.contributor_breadth_worker.contributor_breadth_model) (import-outside-toplevel)

@@ -205,8 +205,10 @@ def setup_periodic_tasks(sender, **kwargs):
from augur.tasks.git.facade_tasks import clone_repos
from augur.tasks.db.refresh_materialized_views import refresh_materialized_views
from augur.tasks.data_analysis.contributor_breadth_worker.contributor_breadth_worker import contributor_breadth_model

with DatabaseEngine() as engine, DatabaseSession(logger, engine) as session:
from augur.application.db import temporary_database_engine
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
C0415: Import outside toplevel (augur.application.db.temporary_database_engine) (import-outside-toplevel)

@@ -233,7 +235,6 @@ def setup_periodic_tasks(sender, **kwargs):
thirty_days_in_seconds = 30*24*60*60
sender.add_periodic_task(thirty_days_in_seconds, contributor_breadth_model.s())


@after_setup_logger.connect
def setup_loggers(*args,**kwargs):
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0613: Unused argument 'args' (unused-argument)

@@ -233,7 +235,6 @@ def setup_periodic_tasks(sender, **kwargs):
thirty_days_in_seconds = 30*24*60*60
sender.add_periodic_task(thirty_days_in_seconds, contributor_breadth_model.s())


@after_setup_logger.connect
def setup_loggers(*args,**kwargs):
Copy link

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0613: Unused argument 'kwargs' (unused-argument)

@ABrain7710 ABrain7710 marked this pull request as ready for review March 9, 2024 15:29
@@ -311,14 +311,14 @@ def create_cache_manager() -> CacheManager:

return cache

def get_server_cache(config, cache_manager) -> Cache:
def get_server_cache(cache_manager) -> Cache:

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0621: Redefining name 'cache_manager' from outer scope (line 732) (redefined-outer-name)

"""Create the server cache, set expiration, and clear

Returns:
server cache
"""

expire = int(config.get_value('Server', 'cache_expire'))
expire = int(get_value('Server', 'cache_expire'))
server_cache = cache_manager.get_cache('server', expire=expire)

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0621: Redefining name 'server_cache' from outer scope (line 733) (redefined-outer-name)

Copy link
Member

@sgoggins sgoggins left a comment

Choose a reason for hiding this comment

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

Lets try it!

@sgoggins sgoggins merged commit b372e98 into dev Mar 11, 2024
6 of 7 checks passed
@ABrain7710 ABrain7710 deleted the config-usage-improvements branch March 15, 2024 00:52
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.

None yet

3 participants