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

Implemented disabling logging configuration & added config file #1130

Merged
merged 10 commits into from
Nov 12, 2020
Merged

Implemented disabling logging configuration & added config file #1130

merged 10 commits into from
Nov 12, 2020

Conversation

briankosw
Copy link
Contributor

@briankosw briankosw commented Nov 10, 2020

Motivation

Implemented disabling the configuration for Python logging, as described in #1021. Added notset.yaml configuration file for hydra/hydra_logging and hydra/job_logging.

Have you read the Contributing Guidelines on pull requests?

(Yes)/No

Test Plan

I tested my implementation locally by comparing the root logger's state before and after hydra.main. I wasn't sure how to write that and add it to the test suite.

Related Issues and PRs

N/A

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 10, 2020
@briankosw
Copy link
Contributor Author

Do you have any suggestions for how tests should be implemented for such changes? What about documentation?

@jieru-hu
Copy link
Contributor

Do you have any suggestions for how tests should be implemented for such changes? What about documentation?

for testing, i think these are good examples

def test_logging(tmpdir: Path) -> None:
cmd = [
"examples/configure_hydra/logging/my_app.py",
"hydra.run.dir=" + str(tmpdir),
]
result, _err = get_run_output(cmd)
assert result == "[INFO] - Info level message"
def test_workdir_config(monkeypatch: Any, tmpdir: Path) -> None:
script = str(Path("examples/configure_hydra/workdir/my_app.py").absolute())
monkeypatch.chdir(tmpdir)
result, _err = get_run_output([script])
assert Path(result) == Path(tmpdir) / "run_dir"
result, _err = get_run_output(
[script, "--multirun", "hydra/hydra_logging=disabled"]
)
assert Path(result) == Path(tmpdir) / "sweep_dir" / "0"

For documentation, the Hydra confs are already linked here https://hydra.cc/docs/next/configure_hydra/intro, so i think it's probably fine not to document it.

@briankosw briankosw marked this pull request as ready for review November 10, 2020 19:27
@jieru-hu
Copy link
Contributor

Thx for working on this! Please fix the lint failures. Other than that, one small comment.
I will leave this to @omry for merging/accepting.

Copy link
Contributor

@shagunsodhani shagunsodhani left a comment

Choose a reason for hiding this comment

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

Some feedback

hydra/conf/hydra/hydra_logging/notset.yaml Outdated Show resolved Hide resolved
tests/test_examples/test_configure_hydra.py Outdated Show resolved Hide resolved
@omry
Copy link
Collaborator

omry commented Nov 11, 2020

Nice.

  1. Add a news fragment.
  2. Mention it in the docs (see bottom part of https://hydra.cc/docs/tutorials/basic/running_your_app/logging).

.vscode/settings.json Outdated Show resolved Hide resolved
@briankosw briankosw requested a review from omry November 11, 2020 11:15
@@ -0,0 +1 @@
This change will allow disabling the configuration of Python's logging subsystem.
Copy link
Contributor

@jieru-hu jieru-hu Nov 11, 2020

Choose a reason for hiding this comment

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

Maybe something like the following? (to be more consistent with the other news entries, also no comma at the end.)

It is now possible to disable Hydra's logging configuration

briankosw and others added 2 commits November 12, 2020 11:28
Co-authored-by: Jieru Hu <jieru.hu@gmail.com>
```commandline
$ python my_app.py hydra/job_logging=disabled
<NO OUTPUT>
```

Logging can be [customized](/configure_hydra/logging.md).
You can also set `hydra/job_logging=none` and `hydra/hydra_logging=none` if you do not want Hydra to configure the logging.
and `hydra/hydra_logging` to `none`
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant line?

Copy link
Contributor Author

@briankosw briankosw Nov 12, 2020

Choose a reason for hiding this comment

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

They're separate things: disabled disables the Python logging system such that any pre-existing loggers will also be disabled, while none disables the logging configuration that Hydra sets up. On another note, I did find that the documentation in general was a bit confusing since certain topics were described multiple times in multiple different places. Perhaps the documentation should be changed in general?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring to this part:

Hydra to configure the logging.
and `hydra/hydra_logging` to `none`

Copy link
Collaborator

@omry omry left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@omry omry merged commit 1a85c32 into facebookresearch:master Nov 12, 2020
@briankosw briankosw deleted the feature/configure_py_logging branch November 12, 2020 10:55
@Huizerd Huizerd mentioned this pull request Dec 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants