-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
Hi Orion! This is a great idea - but I think there's a slightly different setup that will lead to even less code-reuse :)
@@ -253,6 +251,11 @@ def __init__( | |||
} | |||
) | |||
|
|||
def process_left_pane_text(self, args: "DictConfig") -> str: |
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.
hmm, honestly, I think that these 3 lines of code will probably be the same for all subclasses of model_chat, meaning that the lines will need to get repeated in all subclasses. I think it'll lead to less duplicated code if, instead, there's a method called something like format_left_pane_text()
, which you just define as pass
for this superclass
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.
Resolved
parlai/crowdsourcing/tasks/model_chat/task_config/model_opts.yaml
Outdated
Show resolved
Hide resolved
@@ -1,2 +1,3 @@ | |||
blender_90M: > | |||
--model-file zoo:blender/blender_90M/model | |||
|
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.
Nit: extra space
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.
Resolved
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.
I concur with Eric's comments
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.
Looks good, thanks! But make sure to wait until all CI checks pass before merging in :)
CI tests passed, merging in now |
Patch description
Refactors code for processing left_pane_text so code injections can be made on other crowdsourcing tasks
Testing steps
See CircleCI tests