Skip to content

Conversation

@stephanwlee
Copy link
Contributor

tf.logging.* is getting deprecated. We will use absl.logging instead from now on.

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

I don’t have a lot of context on this change, but the technical side
looks good to me.

nfelt
nfelt previously requested changes Dec 3, 2018
Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

I think we'll need a "shim" dependency we can use for the internal build to add the requisite absl logging dep, like "expect_absl_logging_installed", added to all the build targets that use logging. (Mostly it will be replacing the tb/compat/tensorflow rule.) We'll want to check when we sync this internally that internal logging continues to work as expected.

Externally, we should also make absl-py a hard requirement for the pip package now that we're directly depending on it, not just using it via TF or conditionally. Let's use absl-py >= 0.4 since that's the release that includes the absl.flags.argparse_flags support as well, and we'll want to be sure we use the absl flag parsing to get the absl.logging flag control.

@stephanwlee
Copy link
Contributor Author

@nfelt thanks a lot for catching few bugs and the great suggestion.

Btw, with absl.logging, we lost color for warn/error. Is this a big deal? If so, do you know where I can learn more about this? (can't seem to find anything about ansi colors in absl or tf_logging.py)

@nfelt
Copy link
Contributor

nfelt commented Dec 5, 2018

Without looking at any actual code, my guess would be that with the old logging we were using our custom LogFormatter in util.py that did coloration, whereas with the absl.logging we're using their LogFormatter which has the same structured format but doesn't do color. I would need to look at the code to confirm though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nfelt should we just format everything absl or should we change the code so we use tensorboard logger?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure I understand the question, but now that you mention it, it probably would be a good idea to use specific logger instances (like tensorflow uses theirs) rather than the root logger, which makes our logging better behaved if it's embedded into other programs.

We could either just do something like this:

log = logging.getLogger('tensorboard')  # or logging.getLogger(__name__)
...
def foo():
  log.info(...)

Or we could define something like a tensorboard.util.logging module, with a get_logger() function that returns the TB logger. With that arrangement we could just put the fake absl dependency on that module, and most of our code would depend on that module instead. (We could also move this function there.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure I understand the question, but now that you mention it, it probably would be a good idea to use specific logger instances (like tensorflow uses theirs) rather than the root logger, which makes our logging better behaved if it's embedded into other programs.

We could either just do something like this:

log = logging.getLogger('tensorboard')  # or logging.getLogger(__name__)
...
def foo():
  log.info(...)

Or we could define something like a tensorboard.util.logging module, with a get_logger() function that returns the TB logger. With that arrangement we could just put the fake absl dependency on that module, and most of our code would depend on that module instead. (We could also move this function there.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still want to use our own log handler here (and for the cases above)? I thought there was already the absl-provided one that gets installed at the root level. So I'd actually have thought that with this arrangement we might get double logging - does that happen?

Unless our handler is much better than the standard absl one, I'd rather just use that one to simplify things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About what we had discussed offline: it looks like the argument/flag has to be parsed before we can determine whether the flag is set to default. This changes when we can configure logger (I am thinking putting it inside program#configure which is a bit larger scope to do in this PR. I will create a follow up PR for that.

@nfelt nfelt merged commit ca5f917 into tensorflow:master Dec 17, 2018
@stephanwlee stephanwlee deleted the logger branch July 22, 2019 20:45
wchargin added a commit that referenced this pull request Oct 10, 2019
Summary:
TensorBoard used to have an optional dependency on `absl-py`, so #1240
and #1409 were careful to only import `absl` if it was installed. But as
of #1654 TensorBoard always requires `absl-py`, so we can promote the
conditional import to a normal import.

Test Plan:
Running TensorBoard with no arguments still fails as expected; running
with `--logdir` or `--version` still do the right things, too.

wchargin-branch: absl-always
wchargin added a commit that referenced this pull request Oct 11, 2019
Summary:
TensorBoard used to have an optional dependency on `absl-py`, so #1240
and #1409 were careful to only import `absl` if it was installed. But as
of #1654 TensorBoard always requires `absl-py`, so we can promote the
conditional import to a normal import.

Test Plan:
Running TensorBoard with no arguments still fails as expected; running
with `--logdir` or `--version` still do the right things, too.

wchargin-branch: absl-always
nfelt added a commit that referenced this pull request Jan 26, 2021
* set WerkzeugServer to use HTTP/1.1 in a proper non-global way

* remove main.py fallback logic for not having absl; obsoleted by #1654

* add main_lib.global_init() to consolidate main.py init steps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants