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] adding ColorLog override will disable other overrides #1706

Closed
NinjaAtWork opened this issue Jul 8, 2021 · 8 comments · Fixed by #1735
Closed

[Bug] adding ColorLog override will disable other overrides #1706

NinjaAtWork opened this issue Jul 8, 2021 · 8 comments · Fixed by #1735
Assignees
Labels
bug Something isn't working In progress Work in progress
Milestone

Comments

@NinjaAtWork
Copy link

NinjaAtWork commented Jul 8, 2021

Hi
here i follow the basic tutorial and experiment section :
main.py:

from omegaconf import DictConfig, OmegaConf
import hydra

@hydra.main(config_path="conf", config_name="config")
def my_app(cfg: DictConfig) -> None:
    print(OmegaConf.to_yaml(cfg))

if __name__ == "__main__":
    my_app()

config.yaml:

defaults:
  - segment : color
  #- override hydra/job_logging: colorlog
  #- override hydra/hydra_logging: colorlog
volume: 1

segment/color.yaml :
color_num=1
segment/color2.yaml :
color_num=10

experiment/test.yaml :

# @package _global_
defaults:
  - override /segment: color2
volume: 9999

running the
:\python main.py +experiment=test
will return volume:9999
BUT while un-commenting colorlog override in config.yaml the volume returned will be volume:1
why adding colorlog impact the other config ?
btw the colorlog do his job and the colorizing the output

thanks

@NinjaAtWork NinjaAtWork added the bug Something isn't working label Jul 8, 2021
@jieru-hu
Copy link
Contributor

jieru-hu commented Jul 8, 2021

Hi @NinjaAtWork

I created this repro based on the info provided.

This does look like a bug - I could reproduce your example locally. If we run --info defaults-tree on the example app, we can see that the defaults tree are generated differently in two cases.

without override hydra/job_logging in primary config

Defaults Tree
*************
<root>:
  hydra/config:
    hydra/output: default
    hydra/launcher: basic
    hydra/sweeper: basic
    hydra/help: default
    hydra/hydra_help: default
    hydra/hydra_logging: default
    hydra/job_logging: default
    hydra/callbacks: null
    hydra/env: default
    _self_
  config:
    segment: color2
    _self_
    experiment: test

with override hydra/job_logging in primary config

Defaults Tree
*************
<root>:
  hydra/config:
    hydra/output: default
    hydra/launcher: basic
    hydra/sweeper: basic
    hydra/help: default
    hydra/hydra_help: default
    hydra/hydra_logging: colorlog
    hydra/job_logging: colorlog
    hydra/callbacks: null
    hydra/env: default
    _self_
  config:
    segment: color2
    experiment: test
    _self_

For now, to get around this, you can try adding the _self_ keyword to your config.yaml file. link to doc

defaults:
  - _self_
  - /segment: color
  - override /hydra/job_logging: colorlog
  - override /hydra/hydra_logging: colorlog

Please give this a try and let us know if it helps. We will also take a look at this issue and apply fix if needed.

@jieru-hu jieru-hu added this to the Hydra 1.1.1 milestone Jul 8, 2021
@NinjaAtWork
Copy link
Author

NinjaAtWork commented Jul 9, 2021

Hi @jieru-hu

adding _self_ to start of the defaults worked and thanks for the solution and your time for this. 💯
just for fun: i read all the docs , as someone who like architecture more than programing , my brain will prefer skip when processing words which start with underline like _ self _ or __ main __ or same others ,you know 😄
please close this issue after a few days which i will be able to report if something somewhere goes tricky or need help.
kindly , by far ,when i use colorlog , then i am unable to use custom job_logging which is described in docs. its great to be able to switch between save to files and save to prompt in colorlog mode .let me know its solution too .

@jieru-hu
Copy link
Contributor

jieru-hu commented Jul 9, 2021

Hi @NinjaAtWork

Do you mean you want to have both colorlog and somehow also follow this example?

If so, you can certainly create your own logging configuration. You can extend the colorlog's config and override the any field as needed (all the Hydra loggings configurations are passed in to logging.config)

Here is the documentation on how to extend a plugin config https://hydra.cc/docs/patterns/configuring_plugins#extending-plugin-default-config

@Jasha10
Copy link
Collaborator

Jasha10 commented Jul 16, 2021

This does look like a bug - I could reproduce your example locally. If we run --info defaults-tree on the example app, we can see that the defaults tree are generated differently in two cases.

without override hydra/job_logging in primary config

...

with override hydra/job_logging in primary config

...

@jieru-hu The desired behavior is that both cases should look like the first defaults tree, right?

I'm going to start working on a fix for this soon (unless you've already got work in progress :) ).

@omry
Copy link
Collaborator

omry commented Jul 17, 2021

Yes, I think this is what I would expect in this situation for the user's config section:

  config:
    segment: color2
    _self_
    experiment: test

@NinjaAtWork
Copy link
Author

@jieru-hu

my custom job logging as you mentioned but this will disable the color effect :

defaults:
  - colorlog
version: 1
formatters:
  simple:
    format: '[%(levelname)s] - %(message)s'
handlers:
  console:
    class: logging.StreamHandler
    formatter: simple
    stream: ext://sys.stdout
root:
  handlers: [console]

disable_existing_loggers: false

i think this will be linked with that minor bug of override. ill wait for new update and try this again. :)

@jieru-hu
Copy link
Contributor

@NinjaAtWork I'm not sure if this is related to this particular bug. You overrides actually disable the colorlog, in particular this line

  console:
    class: logging.StreamHandler
    formatter: simple
    stream: ext://sys.stdout

I've verified this by copy pasting your config and update formatter: colorlog.

@Jasha10 Jasha10 self-assigned this Jul 19, 2021
@NinjaAtWork
Copy link
Author

@jieru-hu
replacing formatter from simple to colorlog will color the console but also change format from '[%(levelname)s] - %(message)s' to colorlog format which include date and time.
i think when user able to define colorlog as defaults in first line then could change format to something desired
all i need is to do something like this :

formatters:
  colorlog:
    format: '[%(levelname)s] - %(message)s'

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

Successfully merging a pull request may close this issue.

4 participants