Refactor Logging, Add GitHub Graph Backend #2131

ytausch · 2024-02-02T14:32:43Z

Important

This PR is an extension of #2139. #2139 should be reviewed and merged first.

As part of my work on #2123, this PR adds the following changes:

The logger config is now handled centrally in setup_logging instead of configuring each module-level logger individually
Add an --online CLI option that downloads missing dependency graph files from GitHub on demand (uses the new GithubLazyJsonBackend)
Added tests

codecov · 2024-02-02T14:42:24Z

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (e9ad40f) 73.21% compared to head (774aff0) 73.93%.

Files	Patch %	Lines
conda_forge_tick/lazy_json_backends.py	86.60%	15 Missing ⚠️
conda_forge_tick/auto_tick.py	60.00%	2 Missing ⚠️
conda_forge_tick/cli.py	86.66%	2 Missing ⚠️
conda_forge_tick/deploy.py	50.00%	1 Missing ⚠️
conda_forge_tick/pypi_name_mapping.py	50.00%	1 Missing ⚠️
conda_forge_tick/update_prs.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2131      +/-   ##
==========================================
+ Coverage   73.21%   73.93%   +0.72%     
==========================================
  Files          96       97       +1     
  Lines        9265     9435     +170     
==========================================
+ Hits         6783     6976     +193     
+ Misses       2482     2459      -23

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

0xbe7a

As mentioned below, I think supporting writes is pretty straightforward and would allow all changes to be diffed. Otherwise LGTM

README.md

conda_forge_tick/lazy_json_backends.py

xhochy · 2024-02-02T15:26:59Z

@ytausch Can you please separate out the LOGGER/logger changes into a separate PR to uncluter this diff?

beckermr

I am still going through the PR but here are some initial comments.

conda_forge_tick/utils.py

conda_forge_tick/update_prs.py

conda_forge_tick/make_graph.py

tests/test_lazy_json_backends.py

ytausch · 2024-02-02T16:10:41Z

@ytausch Can you please separate out the LOGGER/logger changes into a separate PR to uncluter this diff?

Done in #2132

beckermr

More comments. Sorry they are coming bit by bit. This diff makes non-trivial changes in more than one spot. A few suggestions.

It appears something like isort was run on some of the files? Can we add this to pre-commit in another PR?
Can the changes to the versions code and the lazyjson stuff be in two separate PRs?
Have you run this code live locally? There can be subtle issues with changes in how the code is structured causing new and destructive I/O patterns. Whenever you do any operation on the "payload" object (called "attrs" in much of the code), the lazy json backends will do things like download the json, read it into memory, make calls to a DB etc. So how those operations are structured in the code is pretty important.

conda_forge_tick/cli.py

conda_forge_tick/utils.py

conda_forge_tick/lazy_json_backends.py

beckermr · 2024-02-02T18:55:06Z

conda_forge_tick/update_upstream_versions.py

@@ -1,16 +1,28 @@
-import networkx as nx


These changes are pretty extensive. Is there a way we can add what you need without all of the refactoring here? The bot test suite is not good enough to detect bugs in the new code and I am worried this is going to fail once we run it live.

This discussion now belongs to #2139 - but let me give a short reply here: Of course the refactoring is not necessary but my point is that the code is now more modular, more descriptive, generates more helpful debug output, and can also be tested rather well.

Also, I did not introduce big structural changes so it should be possible to verify the correctness of my changes in a code review. Unit tests now cover the entirety of my changes.

ytausch · 2024-02-05T07:39:29Z

It appears something like isort was run on some of the files? Can we add this to pre-commit in another PR?

Actually it was the automatic Optimize Imports functionality of PyCharm. Since I could not identify a consistent ordering of the imports, I did not pay much attention to it. I will create a new PR now that adds isort as pre-commit hook.

Can the changes to the versions code and the lazyjson stuff be in two separate PRs?

Do you mean separating the logic for updating a single package into another PR? I can do that, sure. As an explanation: I think the GitHub LazyJSON backend only makes sense if used for updating one single package. This is why I introduced both changes at once.

Have you run this code live locally? There can be subtle issues with changes in how the code is structured causing new and destructive I/O patterns. Whenever you do any operation on the "payload" object (called "attrs" in much of the code), the lazy json backends will do things like download the json, read it into memory, make calls to a DB etc. So how those operations are structured in the code is pretty important.

I did not test the code with the MongoDB backend, only with the file backend locally. I don't think my changes should break something if the MongoDB backend adheres to the LazyJsonBackend specification but I will test it nonetheless. It's understandable that you want to be extra-cautious to not break production.

ytausch · 2024-02-12T15:33:31Z

@beckermr Please note that this now depends on #2139

ytausch · 2024-02-12T15:37:37Z

A note about your comment "did you test this live locally": I will now look into the possibility to develop a basic integration test that ensures basic functionality for future changes. This will be a separate PR.

handling of empty/nonexistent graph file

This reverts commit 02101bc.

ytausch · 2024-02-13T17:28:23Z

I ran this though mypy and verified manually that my changes do not introduce new type issues.

beckermr

LGTM! Nice work on the github backend!

jakirkham · 2024-03-06T00:06:05Z

conda_forge_tick/utils.py

-def setup_logger(logger: logging.Logger, level: str = "INFO") -> None:
-    """Basic configuration for logging"""
-    logger.setLevel(level.upper())
-    ch = logging.StreamHandler()
-    ch.setLevel(level.upper())
-    ch.setFormatter(
-        logging.Formatter("%(asctime)-15s %(levelname)-8s %(name)s || %(message)s"),
+def setup_logging(level: str = "INFO") -> None:
+    logging.basicConfig(
+        format="%(asctime)-15s %(levelname)-8s %(name)s || %(message)s",
+        level=level.upper(),
    )
-    logger.addHandler(ch)
-    # this prevents duplicate logging messages
-    logger.propagate = False
+    logging.getLogger("urllib3").setLevel(logging.INFO)


It looks like this change broke the bot

ref: regro/cf-graph-countyfair#19 (comment)

Bot is functioning fine. It broke ci tests on that repo which are there only to double check manual changes.

That's fair. Wrote that kind of tersely

Am not seeing this function used in the org. Are we getting an old package?

ytausch changed the title ~~Refactor Logging, Add GitHub Graph Backend,~~ Refactor Logging, Add GitHub Graph Backend Feb 2, 2024

ytausch force-pushed the local_upgrade branch from a715667 to 99b65b3 Compare February 2, 2024 14:33

0xbe7a reviewed Feb 2, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

conda_forge_tick/lazy_json_backends.py Show resolved Hide resolved

ytausch marked this pull request as draft February 2, 2024 15:08

beckermr requested changes Feb 2, 2024

View reviewed changes

ytausch force-pushed the local_upgrade branch from bf72204 to 61372dc Compare February 2, 2024 18:01

ytausch marked this pull request as ready for review February 2, 2024 18:29

ytausch requested a review from beckermr February 2, 2024 18:29

beckermr requested changes Feb 2, 2024

View reviewed changes

ytausch force-pushed the local_upgrade branch from 6f3c847 to 94fd022 Compare February 8, 2024 17:06

ytausch changed the title ~~Refactor Logging, Add GitHub Graph Backend~~ Refactor Logging, Add GitHub Graph Backend, Single Package Update Feb 9, 2024

ytausch mentioned this pull request Feb 9, 2024

Support Updating Single Package, Refactoring update_upstream_versions #2139

Merged

ytausch marked this pull request as draft February 9, 2024 19:36

ytausch force-pushed the local_upgrade branch from c55bd87 to 1881825 Compare February 12, 2024 10:19

ytausch changed the title ~~Refactor Logging, Add GitHub Graph Backend, Single Package Update~~ Refactor Logging, Add GitHub Graph Backend Feb 12, 2024

ytausch marked this pull request as ready for review February 12, 2024 15:32

ytausch requested a review from beckermr February 12, 2024 15:33

ytausch force-pushed the local_upgrade branch from 4014edc to 755fed1 Compare February 13, 2024 14:48

ytausch added 6 commits February 13, 2024 18:26

mark abstract class as abstract

48e4a65

add online JSON backend

452f182

use logging consistently, configure centrally

ce70ef1

remove unused argument

5716629

add tests and error handling

e1b09d7

Add documentation about local debug functionality.

2edce24

ytausch added 18 commits February 13, 2024 18:26

raise error when graph file not found

008c8e3

remove redundant type checking clause

c5211d3

add !TIP about the --online flag

a32e708

don't group tests into class, test online against GitHub

52f921e

fix: GitHub hexists

ef4253f

avoid the term GitHub API for raw URLs

0c97b2d

use context manager for overriding backends

6d8d426

correct typing for load_graph

3cafa90

handling of empty/nonexistent graph file

add missing import

b9152fc

fix test: load_graph

3ddcf88

gather_all_feedstocks does not take context

2f8b64c

hkeys and hgetall is not implemented in GitHubLazyJsonBackend

f569f63

add tests for GitHub backend

982bb88

add tests for load_graph, document behaviour

d310703

try moving caplog set level down

6e7205b

Revert "try moving caplog set level down"

a6a5b8b

This reverts commit 02101bc.

reset GitHub backend in-between tests

2b6f1bd

fix types

774aff0

ytausch force-pushed the local_upgrade branch from 99fb4a0 to 774aff0 Compare February 13, 2024 17:27

beckermr approved these changes Feb 13, 2024

View reviewed changes

beckermr merged commit 982e137 into regro:master Feb 13, 2024
5 checks passed

ytausch deleted the local_upgrade branch February 14, 2024 10:33

jakirkham reviewed Mar 6, 2024

View reviewed changes

This was referenced Mar 6, 2024

Reset cuda-python in python312 migrator regro/cf-graph-countyfair#19

Merged

BUG fix logger issue conda-forge/webservices-dispatch-action#48

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Logging, Add GitHub Graph Backend #2131

Refactor Logging, Add GitHub Graph Backend #2131

ytausch commented Feb 2, 2024 •

edited

Loading

codecov bot commented Feb 2, 2024 •

edited

Loading

0xbe7a left a comment •

edited

Loading

xhochy commented Feb 2, 2024

beckermr left a comment

ytausch commented Feb 2, 2024

beckermr left a comment

beckermr Feb 2, 2024

ytausch Feb 12, 2024

ytausch commented Feb 5, 2024 •

edited

Loading

ytausch commented Feb 12, 2024

ytausch commented Feb 12, 2024 •

edited

Loading

ytausch commented Feb 13, 2024

beckermr left a comment

jakirkham Mar 6, 2024

beckermr Mar 6, 2024

jakirkham Mar 6, 2024

Refactor Logging, Add GitHub Graph Backend #2131

Refactor Logging, Add GitHub Graph Backend #2131

Conversation

ytausch commented Feb 2, 2024 • edited Loading

codecov bot commented Feb 2, 2024 • edited Loading

Codecov Report

0xbe7a left a comment • edited Loading

Choose a reason for hiding this comment

xhochy commented Feb 2, 2024

beckermr left a comment

Choose a reason for hiding this comment

ytausch commented Feb 2, 2024

beckermr left a comment

Choose a reason for hiding this comment

beckermr Feb 2, 2024

Choose a reason for hiding this comment

ytausch Feb 12, 2024

Choose a reason for hiding this comment

ytausch commented Feb 5, 2024 • edited Loading

ytausch commented Feb 12, 2024

ytausch commented Feb 12, 2024 • edited Loading

ytausch commented Feb 13, 2024

beckermr left a comment

Choose a reason for hiding this comment

jakirkham Mar 6, 2024

Choose a reason for hiding this comment

beckermr Mar 6, 2024

Choose a reason for hiding this comment

jakirkham Mar 6, 2024

Choose a reason for hiding this comment

ytausch commented Feb 2, 2024 •

edited

Loading

codecov bot commented Feb 2, 2024 •

edited

Loading

0xbe7a left a comment •

edited

Loading

ytausch commented Feb 5, 2024 •

edited

Loading

ytausch commented Feb 12, 2024 •

edited

Loading