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

Implement AbstractLogger interface #9

Merged
merged 7 commits into from
Apr 10, 2019
Merged

Implement AbstractLogger interface #9

merged 7 commits into from
Apr 10, 2019

Conversation

PhilipVinc
Copy link
Member

Implements the logging interface.

Can break down arbitrary structs #5 into elementary blocks.

This also exposes an API to allow other packages to edit how TensorBoardLogger logs custom types.

To change how a custom type is logged, preprocess(name::String, value::CustomType, data) should be defined, pushing to data name-value pairs of objects that should be logged. For example, complex numbers are internally handled by the following preprocess function:

preprocess(name, val::Complex, data) = push!(data, name*"/re"=>real(val), 
                                                   name*"/im"=>imag(val))

If someone has a better idea for how to implement this interface, please feel free to share.

src/Logger.jl Outdated Show resolved Hide resolved
@codecov-io

This comment has been minimized.

@oxinabox
Copy link
Member

I have not reviewed this yet, I will try and find time to in the next few days if you would like another set of eyes over it.

It might also be good if @c42f could take a look

@PhilipVinc
Copy link
Member Author

Please do! I would really appreciate a pair (or two) of more experienced eyes on this.

@c42f
Copy link
Member

c42f commented Mar 27, 2019

This one is interesting! As you can probably see, using @info and friends makes TB logging composable without the need to pass the logger around everywhere. On the other hand, TB logging might be a bit outside what I had in mind for the logging system so it will be an interesting (and useful) exercise to see whether it all works properly :-)

Some high level questions, keeping in mind that I haven't had time to read up on what tensorboard can do or experiment with it:

  • What is the expected log structure for tensorboard? Does it have limitations on the structure it can ingest?
  • How is log event information styled by the tensorboard UI?
  • Will log events generated by modules which know nothing about tensorboard work ok?

The system as a whole needs to map structured log events into something which will be styled and presented via the tensorboard UI. Log events generated with @info etc should have structure but no style information. I guess I'm wondering how the style gets in there.

@oxinabox
Copy link
Member

oxinabox commented Mar 27, 2019

How is log event information styled by the tensorboard UI?

Dispatch on the type of the thing being logged.

@PhilipVinc
Copy link
Member Author

What is the expected log structure for tensorboard?
Will log events generated by modules which know nothing about tensorboard work ok?

Kinda-yes. Tensorboard essentially takes tags and data. Right now when a message is logged I generate the tag by combining the message and the key associated to the data. So @info "observables" magnetization=12 will be logged under the tag observables/magnetization with value 12. If you have more complicated strings in the message part of the logmsg this would both look terribly wrong in the Tensorboard UI and probably need some string sanitization.

Does it have limitations on the structure it can ingest?

If we implement a String logging fallback, I don't see why it should have limitations. Plus package authors could use the preprocess function to specify what fields of a struct to drop out so that only useful thing are logged.

@oxinabox
Copy link
Member

oxinabox commented Mar 27, 2019

What to do with complex messages is unclear to me also. should open another issue to discuss that. Or work it out when we get up to it.
Probably when just a message is given it should go straight too text.

@c42f
Copy link
Member

c42f commented Mar 28, 2019

Ok thanks. It sounds like tensorboard actually has quite a good match of concepts with the logging system. That's fortunate!

How is log event information styled by the tensorboard UI?

Dispatch on the type of the thing being logged.

Cool I think this is the right thing and will lead to reasonable defaults. It's pretty much the same conclusion I came to with Logging.ConsoleLogger.

Copy link
Contributor

@shashikdm shashikdm left a comment

Choose a reason for hiding this comment

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

automatic dispatch of logger functions using summary_impl is very elegant but there will be ambiguity when we implement log_image since both log_image and log_histogram should be able to take 2-D or 3-D arrays. So users should have the freedom to specify which logger they want to use when logging certain object. When user does not specify any particular logger, then defaults can be used.

src/Logger.jl Show resolved Hide resolved
  - preprocess now calls itself recursively and only pushes to a list if the type can be serialized.
  - Remove the `loggable` function as now it's no longer needed.
  - There is also no need for a stack (and the DataStructures dependency). Just use a Vector for `data` in `handle_message`
  - Fix the readme: `delta_step` -> `log_step_increment`
@PhilipVinc
Copy link
Member Author

@shashikdm Yes. That's something that we should discuss in #10.

I believe we can always define some wrapper type like

struct TBImage{T} where T
 data::T
end

and use this to propagate the fact that we want to serialize such data as an image. If the default behavior is defined in preprocess, then one can override it by redefining it. We could even provide an helper function to do it.

@PhilipVinc
Copy link
Member Author

@oxinabox Any additional comments? Looks fine by you?

src/Logger.jl Outdated Show resolved Hide resolved
@c42f
Copy link
Member

c42f commented Apr 5, 2019

both log_image and log_histogram should be able to take 2-D or 3-D arrays

I suggest the right way to fix this is a wrapper type for arrays which tells the logging backend of the semantic of the array.

For example, just present normal 2D arrays as images; if you want a histogram as presentation I think this implies you're interpreting the array as samples from some distribution, and you should wrap the array appropriately in a Samples wrapper. (Choose a better name of course ;-) ).

As a very rough illustration:

my_samples = randn(100,10)
@info "Blah" Samples(my_samples)

Then you encode the interpretation of my_samples in the type for later dispatch, but you don't explicitly mention that this will become a histogram when displayed. (For example, a text-based representation may way to write some summary statistics instead.)

@oxinabox
Copy link
Member

oxinabox commented Apr 8, 2019

Don't wait for me before merging, I think this is good to get started with and can always change as problems occur?

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.

5 participants