Reapply on_config_parsed after applying cli args#1448
Reapply on_config_parsed after applying cli args#1448karriebear merged 7 commits intostreamlit:developfrom
Conversation
karriebear
force-pushed
the
bug/1321-setting-logLevel
branch
from
5a97215 to
bf520e4
Compare
karriebear
force-pushed
the
bug/1321-setting-logLevel
branch
3 times, most recently
from
ac854bd to
9f693e9
Compare
karriebear
force-pushed
the
bug/1321-setting-logLevel
branch
from
9f693e9 to
9e7b757
Compare
| # avoids a race condition when another file (such as a test file) tries to pass | ||
| # in an alternative config. | ||
| _config.on_config_parsed(_set_log_level) | ||
| _config.on_config_parsed(_set_log_level, True) |
There was a problem hiding this comment.
Q: Why force the connect_signal?
A: When we hit this, we've already parsed the config once, and we need set_log_level to be re-run when we parse the cli args and call signal.send()
There was a problem hiding this comment.
We should find out if there is a definitive ordering between the config.toml being parsed and the cli args being parsed. I think it impacts our logic.
There was a problem hiding this comment.
Right now config.toml is being parsed as a result of getting global.developmentMode in folder_black_list.py which is imported through a series of imports.
cli.py --> bootstrap.py --> caching.py --> hashing.py
Given the current imports, it's definitive but it isn't exactly straightforward. If a change were to be made, I don't think it'd be intuitive to know.
Updated cli.py to force a definitive ordering.
lib/streamlit/cli.py
Outdated
| "command-line argument or environment variable", | ||
| ) | ||
| if _config._config_file_has_been_parsed: | ||
| _config._on_config_parsed.send() |
There was a problem hiding this comment.
Is it possible for _config_file_has_been_parsed to be False here?
If so, are we still overriding config.toml args or will they now override the CLI args?
There was a problem hiding this comment.
If it is True, is it ok to execute the connected functions multiple times?
They should have executed initially after setting that to True
There was a problem hiding this comment.
Potential idea: If config.toml hasn't been parsed when we get here, force it
There was a problem hiding this comment.
Updated to force config file to be parsed prior to applying CLI args.
Current connected functions are _check_conflicts and _set_development_mode. Both of these seem fine to (and probably should) run again after applying CLI args.
karriebear
force-pushed
the
bug/1321-setting-logLevel
branch
from
b089e14 to
5b5e2f3
Compare
| # goes out of scope immediately. | ||
| _on_config_parsed.connect(lambda _: func(), weak=False) | ||
| else: | ||
| func() |
There was a problem hiding this comment.
I think there's something better we need to do with the config system, which requires more thought.
But in the meantime, can we refactor this current solution for readability?
For example, we could have two signals:
if file has not been parsed:
_on_config_parsed.connect(lambda _: func(), weak=False)
if command-line args have not been parsed:
_on_args_parsed.connect(lambda _: func(), weak=False)4 participants
Issue: Fixes #1321
Description:
set_log_levelruns onceon_config_parsedis emitted. This can get triggered before the CLI arguments are parsed. Following changes to address this:on_config_parsed.on_config_parsedwith a flag to forcefully connect the callback with the signal. Currently a shortcut exists to bypass connecting to the signal.Contribution License Agreement
By submiting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.