Skip to content

ioana.circu - logging #4

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 10 commits into from
Apr 7, 2025
Merged

ioana.circu - logging #4

merged 10 commits into from
Apr 7, 2025

Conversation

lr154lrose
Copy link
Collaborator

Added logger

  • Option to disable logging
  • Option to log to console or only to file

- setup function in separate file
- added option to not log the output if required
- added extra two lines in dirconfig for the log file location
@lr154lrose lr154lrose requested a review from dwest77a March 14, 2025 09:57
@@ -0,0 +1,45 @@
import logging

def setup_logging(enable_logging=True, console_logging=True):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding this comment here but it can apply in many places. Consider using type hints for the parameters of functions. In this case enable_logging: bool = True etc.

If multiple variable types are acceptable, you can use the following:

from typing import Union
...
enable_logging: Union[bool,None] = None

:param enable_logging: Flag to enable/disable logging.
"""

f = open('dirconfig','r')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can use the updated syntax for opening files:

with open(file) as f: # 'r' is default if not specified.
    content = [r.strip() for r in f.readlines()] # Removes the '\n' from all lines


settings_default = {
"hosts": [
"https://es10.ceda.ac.uk:9200"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can just use elasticsearc.ceda.ac.uk now.


self.es = Elasticsearch(**connection_kwargs)

def preprocess_records(self, records):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Look into python Abstract Base Classes at https://www.geeksforgeeks.org/abstract-classes-in-python/

This is something Dave suggested but I know less about. The preprocess_records function is overridden by subclasses (not sure where), which is exactly the use case for an ABC class.

@@ -22,6 +25,7 @@
urllib3.disable_warnings()

def resolve_link(path, ):
logger.debug("Debug: Resolving link for path %s", path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need the Debug: part of the string if your log message already adds this.

@dwest77a
Copy link
Collaborator

Few additional comments for suggested changes to other parts of the code:

  • Put all .py files inside the flightpipe module (then they can be part of the main package)
  • Any script you would normally use like python <script_name> can then instead use an entrypoint in the pyproject.toml file project.scripts section (see cci-os-worker)
  • Any new python scripts you create, including the cli.py file - add the sections below to the file:
__author__    = "Ioana Circu"
__contact__   = "[email protected]"
__copyright__ = "Copyright 2025 United Kingdom Research and Innovation"

Along with a copy of the License file from another project that sits in the top of the repository.

@dwest77a
Copy link
Collaborator

dwest77a commented Apr 4, 2025

Great, looks like all the logging changes are fine and working so you can merge this branch into main now!

@lr154lrose lr154lrose merged commit 45f2fa2 into main Apr 7, 2025
1 check passed
@lr154lrose lr154lrose deleted the origin/ioana.circu-logging branch April 10, 2025 13:42
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