Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

context manager that allows predictor to capture model internals #2581

Merged
merged 6 commits into from
Mar 13, 2019

Conversation

joelgrus
Copy link
Contributor

@joelgrus joelgrus commented Mar 8, 2019

as I said, this approach is slightly on the "cute" side, but I like how utterly non-intrusive it is

@matt-gardner
Copy link
Contributor

I think this is pretty slick. I have no complaints with this approach. One issue I see in using this in a demo is deciding which things to show to the user. That will have to be configured somewhere (e.g., you might consider taking a filter function or something as an argument to the context manager). But getting the internals this way seems great.

@matt-gardner
Copy link
Contributor

There's also the issue of wanting to get internals that aren't the result of submodules, but this is definitely an improvement over the current situation. Most things we would want to include in a demo are probably the output of a module, anyway.

Copy link
Contributor

@matt-gardner matt-gardner left a comment

Choose a reason for hiding this comment

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

Looks great!

# If you capture the return value of the context manager, you get the results dict.
yield results

# And then when you exit the context we remove all the hooks.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for these comments - I'm not familiar with how context managers work, so this helps a lot.

@joelgrus joelgrus merged commit b61d511 into allenai:master Mar 13, 2019
reiyw pushed a commit to reiyw/allennlp that referenced this pull request Nov 12, 2019
…enai#2581)

* model internals

* undo formatting change

* more undo formatting

* work

* mypy + docs
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants