cmu-delphi / covidcast-indicators

Back end for producing indicators and loading them into the COVIDcast API.
https://cmu-delphi.github.io/delphi-epidata/api/covidcast.html
MIT License
12 stars 17 forks source link

Fix unstructured logging variable formatting #2029

Closed aysim319 closed 1 week ago

aysim319 commented 1 month ago

Description

replacing logging with f strings with parameters

Associated Issue(s)

aysim319 commented 1 month ago

looks pretty good! i marked a few things i think you mightve missed, and tried to change filename= where it actually referred to a directory and not a singular file.

Ah yeah, that was an intentional choice. I didn't see any output_path being used and it techincally also has the filename so whynot and worst comes to worst someone tells me it's not right and i'd fix it.

melange396 commented 1 month ago

im gonna set this to "draft" and then back again to try to get ci to re-run

melange396 commented 1 month ago

It looks like there were 3 files from delphi_utils mentioned in #2025 that you havent gotten to yet.

melange396 commented 1 month ago

This PR is a great opportunity to add the text from my suggestion in https://github.com/cmu-delphi/covidcast-indicators/issues/2041#issuecomment-2319002584 -- the changes you made here exemplify the intent of it, and the code replaced embodies some anti-patterns... That way, it will all be bundled together for posterity.

aysim319 commented 2 weeks ago

./changehc/delphi_changehc/sensor.py:121: logger.debug("{0}: {1:.3f},[{2:.3f}]".format( ./changehc/delphi_changehc/update_sensor.py:52: logger.warning("value suspiciously high, {0}: {1}".format( ./changehc/delphi_changehc/update_sensor.py:149: self.logger.error("{0} is invalid, pick one of 'county', " ./doctor_visits/delphi_doctor_visits/sensor.py:242: logger.debug(f"{geo_id}: {new_rates[-1]:.3f},[{se[-1]:.3f}]") ./claims_hosp/delphi_claims_hosp/indicator.py:146: logging.debug("%s: %05.3f, [%05.3f]

The second one is one that I missed and third one, I didn't feel like parameterizing a wrong value made sense, but I could also see why it would be helpful...

But for the rest, I think it's better to leave it as is as they're debugging logs where I think seeing the message as is seems more helpful instead of having it parameterized.

Edited changed all of the instance

aysim319 commented 2 weeks ago

@dshemetov since you and david is planning a formatting PR, thought I ping you about this getting inconsistent behavior for darker: similar isssue: https://github.com/psf/black/issues/1140

in _delphi_utils.delphi_utils.runner.py line 63-66 and 82: local darker wants one line

# Logging - Starting Indicator
        logger.info("Started a covidcast-indicator", indicator_name=ind_name, current_version=current_version)
    else:
        logger.info("Started a covidcast-indicator without version.cfg", indicator_name=ind_name)      

logger.error("Flash step timed out, terminating", elapsed_time_in_seconds=round(time.time() - start, 2))

CI wants multi-line

@@ -62,11 +62,11 @@
                     current_version = current_version.replace("current_version = ", "")
     #Logging - Starting Indicator
         logger.info(
             "Started a covidcast-indicator",
             indicator_name=ind_name,
-            current_version=current_version
+            current_version=current_version,
         )
     else:
         logger.info(
             "Started a covidcast-indicator without version.cfg", indicator_name=ind_name
         )
@@ -83,13 +83,14 @@
             logger.info("Completed flash step",
                     elapsed_time_in_seconds = round(time.time() - start, 2))
             break
         time.sleep(1)
     else:
-        logger.error("Flash step timed out, terminating",
-                     elapsed_time_in_seconds=round(time.time() - start, 2)
-                 )
+        logger.error(
+            "Flash step timed out, terminating",
+            elapsed_time_in_seconds=round(time.time() - start, 2),
+        )
dshemetov commented 2 weeks ago

Hm, is it resolved now or did you have to manually adjust it to make CI happy? I might need to point the format commands in both CI and Makefiles to the pyproject.toml just to eliminate the possibility that they're not finding it (it specifies line-length = 120 there and this seems like a disagreement about line-length).

aysim319 commented 2 weeks ago

Hm, is it resolved now or did you have to manually adjust it to make CI happy? I might need to point the format commands in both CI and Makefiles to the pyproject.toml just to eliminate the possibility that they're not finding it (it specifies line-length = 120 there and this seems like a disagreement about line-length).

I had to manually adjust it .... :/

melange396 commented 1 week ago

@aysim319 i removed some stray files that somehow wandered into your last commit, and i changed the format of a reverted logger call back to make it pass linting.