Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

[eval model] store world logs per task #3718

Merged
merged 3 commits into from
Jul 7, 2021
Merged

[eval model] store world logs per task #3718

merged 3 commits into from
Jul 7, 2021

Conversation

jxmsML
Copy link
Contributor

@jxmsML jxmsML commented Jun 14, 2021

Patch description
Currently when running parlai em --world-logs ..... it only stores the world logs for the very last task if multitasking.
This pr would fix this issue by adding the task name as suffix to the world log file path if it's multitasking.

Fixes #3648

Testing steps

.(conda_parlai) jingxu23@devfair0173:~/ParlAI$ python /private/home/jingxu23/ParlAI/tests/test_transformers.py

.....
----------------------------------------------------------------------
Ran 34 tests in 149.635s

OK

Other information

@jxmsML jxmsML requested a review from stephenroller June 14, 2021 22:42
@jxmsML jxmsML changed the title log worlds per task [eval model] store world logs per task Jun 14, 2021
@stephenroller
Copy link
Contributor

Please add a test, and also merge the entire bits into the f-string, rather than using concatenation

@jxmsML jxmsML force-pushed the eval_log branch 2 times, most recently from e672924 to d79f19d Compare July 7, 2021 15:44
Copy link
Contributor

@stephenroller stephenroller left a comment

Choose a reason for hiding this comment

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

Tyvm

@jxmsML jxmsML merged commit e357c94 into master Jul 7, 2021
@jxmsML jxmsML deleted the eval_log branch July 7, 2021 17:01
return world_logs
else:
base_outfile, extension = os.path.splitext(world_logs)
return f'{base_outfile}_{task}{extension}'
Copy link
Contributor

@kauterry kauterry Jul 8, 2021

Choose a reason for hiding this comment

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

Shouldn't it be this?

return f'{base_outfile}_{task}.{extension}'

Copy link
Contributor

Choose a reason for hiding this comment

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

No, splitext includes the . In the ext

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, sorry!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the extension contains "."

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

Successfully merging this pull request may close these issues.

Using world-logs while evaluating a model with more than one task, only keeps the world log for the last task.
4 participants