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

Improve tabulate #2316

Merged
merged 1 commit into from
Aug 29, 2022
Merged

Improve tabulate #2316

merged 1 commit into from
Aug 29, 2022

Conversation

cgarciae
Copy link
Collaborator

@cgarciae cgarciae commented Jul 21, 2022

What does this PR do?

Overhauls tabulate with a new system to capture call information and fixes #2274 and #2359.

Changes

  • Adds a new thread-local _CallInfoContext context manager that provides richer call information.
  • Table entries are now sorted by call order which is more intuitive / correct.
  • Two new columns are added to the table:
    • module: shows module type
    • inputs: shows input information
  • Removes the exclude_methods arguments, it is no longer necessary as capture_intermediates is no longer used.
  • A new show_repeated arguments is added, lets you decide if you want to show repeated modules in case the same module is called multiple times.
  • Adds a new console_kwargs argument that lets users configure the rich.console.Console object used to render the table.

Future issues

Some issues related to Scope.path where uncovered here, will post issues with a minimal repro in the near future. To get around these issues some temporal fixes where used, see TODOs associated with each issue.

  • Scope.path sometimes empty when a module is called more than once, see TODO.
  • Scope.path names sometimes don't match the variable structure, specific issue found when naming a lifted Module, see TODO.

Sample

Screenshot from 2022-08-08 18-31-03

@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2022

Codecov Report

Attention: Patch coverage is 97.83784% with 4 lines in your changes missing coverage. Please review.

Project coverage is 70.00%. Comparing base (f754541) to head (b1025cc).
Report is 1757 commits behind head on main.

Files with missing lines Patch % Lines
flax/linen/module.py 96.22% 2 Missing ⚠️
flax/linen/summary.py 98.48% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2316      +/-   ##
==========================================
+ Coverage   69.33%   70.00%   +0.67%     
==========================================
  Files          63       63              
  Lines        5556     5664     +108     
==========================================
+ Hits         3852     3965     +113     
+ Misses       1704     1699       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cgarciae cgarciae marked this pull request as ready for review July 21, 2022 21:32
@cgarciae cgarciae requested a review from jheek July 25, 2022 21:45
@andsteing andsteing added the Priority: P1 - soon Response within 5 business days. Resolution within 30 days. (Assignee required) label Jul 26, 2022
flax/linen/module.py Outdated Show resolved Hide resolved
flax/linen/module.py Outdated Show resolved Hide resolved
flax/linen/module.py Outdated Show resolved Hide resolved
tests/linen/summary_test.py Outdated Show resolved Hide resolved
tests/linen/summary_test.py Outdated Show resolved Hide resolved
tests/linen/summary_test.py Show resolved Hide resolved
flax/linen/summary.py Outdated Show resolved Hide resolved
flax/linen/module.py Outdated Show resolved Hide resolved
flax/linen/module.py Outdated Show resolved Hide resolved
flax/linen/module.py Outdated Show resolved Hide resolved
flax/linen/module.py Outdated Show resolved Hide resolved
flax/linen/summary.py Outdated Show resolved Hide resolved
flax/linen/summary.py Show resolved Hide resolved
flax/linen/summary.py Show resolved Hide resolved
flax/linen/summary.py Show resolved Hide resolved
flax/linen/summary.py Outdated Show resolved Hide resolved
flax/linen/summary.py Outdated Show resolved Hide resolved
@cgarciae cgarciae force-pushed the improve-tabulate branch 2 times, most recently from 4316d48 to 8148149 Compare August 9, 2022 00:07
@cgarciae cgarciae requested a review from jheek August 9, 2022 14:38
flax/linen/module.py Outdated Show resolved Hide resolved
flax/linen/module.py Outdated Show resolved Hide resolved
flax/linen/module.py Outdated Show resolved Hide resolved
flax/linen/module.py Outdated Show resolved Hide resolved
flax/linen/module.py Outdated Show resolved Hide resolved
Copy link
Member

@jheek jheek left a comment

Choose a reason for hiding this comment

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

Looking good!

@marcvanzee
Copy link
Collaborator

@jheek Something seems to have gone wrong with this PR internally, could you please take a look?

@copybara-service copybara-service bot merged commit 14dffa6 into google:main Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: P1 - soon Response within 5 business days. Resolution within 30 days. (Assignee required) pull ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nn.tabulate results in KeyError: 'intermediates' with methods that include transformations
5 participants