-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Allow self-chats in nested task folders #3785
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.
This seems reasonable? Any way we can add some tests? Maybe just refer to the public ones via absolute paths?
|
||
task = task_path_list[0].lower() |
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.
Wtf why did we lower it
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.
Convention, probably? Namely, none of our task folder names (convai2
, blended_skill_talk
, etc.) are capitalized
parlai/core/loader.py
Outdated
module_name = task_path_list[0] | ||
else: | ||
task = task_path_list[0].lower() | ||
module_name = "%s.tasks.%s.worlds" % (repo, task) |
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.
Can we upgrade to f-strings while we're at it
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.
Yes, changed
Patch description
Currently, it doesn't seem like we can run a self-chat world if the value of
--task
has a'.'
in it, because the code to specify a self-chat world is parsed after we return fromload_world_module()
in that case. This PR shifts that logic around to not fall back on the default world before checking if we want to do self-chats.Testing steps
CI checks, as well as checking that the following command (referring to an internal task) is now possible due to this change:
Also, added testing for the self-chat and interactive mode for a long-form task string