feat(ci): added a network debug report#44636
Conversation
|
Another report example |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
ef75dfc to
67c6933
Compare
There was a problem hiding this comment.
Very nice!
Have a nit question about env. name, and if we really need to have that many functions being public, but those questions are not blocking.
One question I asked myself and then ask Claude is:
Could we minimize the changes in conftest file: the answer is yes but require some design changes. I won't request to change the design in this PR, but just share what I am told here if you are also interested.
Another possibility (AI generated content)
Honestly, they are justified **given the current design** — conftest needs to orchestrate the xdist coordination itself across multiple hooks, so it necessarily needs multiple entry points into `network_logging`.
But the deeper question is whether this design is the right one. An alternative would be a **plugin class** inside `network_logging.py`:
```python
class NetworkDebugPlugin:
def pytest_configure(self, config): ...
def pytest_configure_node(self, node): ...
def pytest_sessionfinish(self, session): ...
def pytest_terminal_summary(self, terminalreporter): ...
Then conftest would shrink to just:
# conftest.py
from transformers.utils.network_logging import register_network_debug_plugin
def pytest_configure(config):
...
register_network_debug_plugin(config) # ← single call, plugin handles everythingAnd register_network_debug_plugin would do:
def register_network_debug_plugin(config):
if _parse_debug_network_env()[0]:
config.pluginmanager.register(NetworkDebugPlugin())The advantages would be:
- Public API shrinks to one function
- All xdist coordination logic lives in one place inside
network_logging.pyinstead of being spread across conftest hooks - conftest stays clean and doesn't need to know about xdist details at all
So the 5 functions are a consequence of the design choice to let conftest drive the orchestration, rather than encapsulating it inside network_logging.py itself. Either approach works, but the plugin class would be cleaner.
|
|
||
| from transformers.utils.network_logging import ( | ||
| clear_network_debug_report, | ||
| disable_network_debug_report, |
There was a problem hiding this comment.
So not used in conftest but for testing.
Kind justify it being public 👍
Thanks for the review. I agree that, since this lives under I initially assumed this would remain internal to our CI usage, so I didn’t optimize for that, but your point makes sense. The plugin-based approach sounds like a cleaner direction for that reason, so I’ll rework the patch to encapsulate more of the orchestration in |
5aaba88 to
0d13a9f
Compare
What does this PR do?
httpxtracer to gather metrics about network callsDEBUG_NETWORKexample of local run: