-
Notifications
You must be signed in to change notification settings - Fork 2.1k
cache api calls and responses on setup #4565
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic looks right based on what we chatted about; couple of coding/safety nits.
Out of curiosity, are the numbers in the Summary from the first time loading the dataset or using the cache for subsequent calls?
76.7 seconds on first pytest run with no presaved cache files |
..and which datatype is that? |
unit tests cover all three looks like |
Ahhh brainfart there; eyes somehow skipped "pytest" there in "first pytest run" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 tiny nits then this looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
* cache api calls and responses on setup * use PathManager instead of os and json instead of pickle * fix json.load and file open * fix typo Co-authored-by: Martin Corredor <mcorredor@devfair0237.h2.fair>
Patch description
Cache api calls to responses dictionary on setup for multiwoz_v22
Testing steps
Unit tests all pass
Train dataset output validation:
Original output on left, new output on right
Test dataset output:
Valid dataset output:
Other information
Unit tests run in 1/4 the time with caching vs without
~63 seconds vs ~257 seconds