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

closes issue #19 #23

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

grassesi
Copy link
Collaborator

@grassesi grassesi commented Aug 1, 2024

import logger as it is setup in utils.logger module and uses logger.info() where ever print is used.
Commented out prints were converted into logger.debug(). Also imports unused imports were removed throughout. For core.train.py, core.train_multi.py and core.evaluator.py the convention to import only modules was enforced for imports from utils.utils. closes #19 .

@grassesi grassesi requested a review from clessig August 1, 2024 13:47
@clessig clessig self-assigned this Aug 6, 2024
@clessig
Copy link
Owner

clessig commented Aug 7, 2024

Hi @grassesi , many thanks for getting started at this. I just tried to run the changes and I get a long list of errors like this:

--- Logging error ---
Traceback (most recent call last):
File "/usr/local/apps/python3/3.10.10-01/lib/python3.10/logging/init.py", line 1100, in emit
msg = self.format(record)
File "/usr/local/apps/python3/3.10.10-01/lib/python3.10/logging/init.py", line 943, in format
return fmt.format(record)
File "/lus/h2resw01/hpcperm/nacl/research/atmorep/develop/atmorep/atmorep/utils/logger.py", line 14, in format
return super().format(record)
File "/usr/local/apps/python3/3.10.10-01/lib/python3.10/logging/init.py", line 678, in format
record.message = record.getMessage()
File "/usr/local/apps/python3/3.10.10-01/lib/python3.10/logging/init.py", line 368, in getMessage
msg = msg % self.args
TypeError: not all arguments converted during string formatting
Call stack:
File "/lus/h2resw01/hpcperm/nacl/research/atmorep/develop/atmorep/atmorep/core/train.py", line 237, in
train()
File "/lus/h2resw01/hpcperm/nacl/research/atmorep/develop/atmorep/atmorep/core/train.py", line 227, in train
cf.print()
File "/lus/h2resw01/hpcperm/nacl/research/atmorep/develop/atmorep/atmorep/utils/utils.py", line 66, in print
logger.info("{} : {}", key, value)

Is there a specific python version of package that is required?

@grassesi
Copy link
Collaborator Author

grassesi commented Aug 12, 2024

Hi @clessig, it should be fixed now.
By default logging only supports old %s style formatting, but i used modern "{}".format style.
To fix this I added support in utils.logger.py by using logging.setLogRecordFactory to set a custom LogRecord that supports both formatting styles. I added unit tests to test this behavior.

@grassesi
Copy link
Collaborator Author

btw currently all logging output is directed towards sys.stderr (default behavior of StreamHandler), so it probably wont show up in the slurm output file. Is this inteded, or should we discuss what output we actually need in which file ?

@grassesi grassesi force-pushed the grasse-19-consistently-use-proper-logging branch from 00ce96c to e773fd4 Compare August 14, 2024 07:55
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