slot extractions v2 #338
slot extractions v2 #338
Conversation
wochinge
force-pushed
the
slot-extractionsv2
branch
from
220a18c
to
ab241bb
Compare
| warnings.warn( | ||
| f"Cannot extract `{slot_name}`: make sure the extract method " | ||
| f"returns the correct output." | ||
| ) |
There was a problem hiding this comment.
Hmmm, I think this warning shouldn't be here. For any user utterance, we try to extract all slots from it. So it's natural that in most cases the call to an extractor won't actually extract anything, right?
There was a problem hiding this comment.
They need to return a dict of the form {"slotname": <value>} so the warning makes sense. Will adapt it a bit though.
There was a problem hiding this comment.
Okay, thanks for correcting me!
|
|
||
| async def required_slots( | ||
| self, | ||
| slots_mapped_in_domain: List[Text], |
There was a problem hiding this comment.
@wochinge what do you think about renaming this to slots_with_predefined_mappings or similar? I'm not 100% certain and would like to get someone else's opinion, but it's true that in the docs I've been trying to remove the mentions of slots "mapped in the domain".
There was a problem hiding this comment.
I think in the documentation it made sense in combination with the link. I think in this case domain makes it clearer for now.
Co-authored-by: Sam Sucik <s.sucik@rasa.com>
wochinge
requested review from
samsucik and
joejuzl
and removed request for
alwx
wochinge
force-pushed
the
slot-extractionsv2
branch
from
0b560d3
to
e58f572
Compare
| ) | ||
| return [] | ||
|
|
||
| if not extract_method: |
There was a problem hiding this comment.
For these conditionals I find it hard to determine which combination of also_mapped_in_domain and extract_method lead to each branch as it requires keeping each previous if in your head. I think it would be clearer if you used either a traditional nested if structure, or re-stated both variables in each flat if statement.
wochinge
force-pushed
the
slot-extractionsv2
branch
from
7bef722
to
c8b8aca
Compare
Proposed changes:
required_slotsreceive the slots which were mapped in the domain as parameter to make it explicit for users that this is something whichrequired_slotsshould returnStatus (please check what you already did):
black(please check Readme for instructions)