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

[Bug] root logger has FileHandler after using hydra_task_runner #833

Closed
nai62 opened this issue Jul 29, 2020 · 5 comments · Fixed by #909
Closed

[Bug] root logger has FileHandler after using hydra_task_runner #833

nai62 opened this issue Jul 29, 2020 · 5 comments · Fixed by #909
Labels
bug Something isn't working
Milestone

Comments

@nai62
Copy link

nai62 commented Jul 29, 2020

🐛 Bug

Once hydra_task_runner is called from a unit test, a FileHandler seems to be registered to the root logger of logging. When a logger tries to log something from another test, it fails because of the staled FileHandler.

I could successfully make the tests pass by manually removing the FileHandler, i.e., adding getLogger().handlers.pop() at the end of test_1(). But it doesn't seem to be an elegant solution. Probably is it possible to remove additional handlers in TaskTestFunction.__exit__() or somewhere?

To reproduce

from logging import getLogger


def test_1(hydra_restore_singletons, hydra_task_runner):
    with hydra_task_runner(
        calling_file="test_hydra.py",
        calling_module=None,
        config_path=".",
        config_name="config.yaml",
    ):
        logger = getLogger(__name__)
        logger.info('test_1')


def test_2():
    logger = getLogger(__name__)
    logger.info('test_2')

Then run pytest.

** Stack trace/error message **

E       FileNotFoundError: [Errno 2] No such file or directory: '/tmp/[TEMPORARY DIRECTORY]/test_hydra.log'

System information

  • Hydra Version : I tried both 1.0.0rc2 and master.
  • Python version : 3.8.3
@nai62 nai62 added the bug Something isn't working label Jul 29, 2020
@omry
Copy link
Collaborator

omry commented Jul 29, 2020

Thanks for reporting.
Can you share more info about your what you are using the task_runner fixture for?
Up to this point it was only used by Hydra itself and Hydra plugins.

@nai62
Copy link
Author

nai62 commented Jul 30, 2020

Actually I wanted to unit test my app that uses hydra. I first tried Compose API and found that hydra.utils.get_original_cwd() failed with them. So I thought it might be better to initialize hydra as much similar way to actual training as possible, and tried to use hydra_task_runner while patching TaskTestFunction.__call__. Is there any better way?

@omry
Copy link
Collaborator

omry commented Jul 30, 2020

  1. The compose API very intentionally does not change the working directory. so hydra.runtime.cwd is not even defined except for ".".

  2. get_original_cwd() is also relying on a a Hydra config singleton which is not populated in compose.
    You could initialize the HydraConfig singleton yourself. I don't have a code sample but look how it's done normally in the codebase.

You can pass return_hydra_conf=true to compose to get the Hydra config node.

@nai62
Copy link
Author

nai62 commented Jul 30, 2020

Thank you for your kind explanation! I didn't notice HydraConfig singleton has to be initialized. I'll give it a try.
So it seems I don't need hydra_task_runner anymore. May I close this issue?

@omry
Copy link
Collaborator

omry commented Jul 30, 2020

Well, normally it is initialized automatically by @hydra.main(). It just occurred to me after seeing your question that initializing it might be a solution in your situation.

The problem you are seeing is likely due to the TaskTestFunction shutting down the logging on exit. During the run of the task the logging subsystem is initialized and it's shutdown after the task.
I need to think about it a bit to see if there is a way around it. so far this has not been an issue for the tests in Hydra.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants