-
Notifications
You must be signed in to change notification settings - Fork 399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Regex
generation method
#175
Conversation
434f9aa
to
2c72f46
Compare
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.
@rlouf, I added the EOS handling and padding.
2c72f46
to
e1f8c55
Compare
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.
FYI: The current error in CI looks like a "determinism" problem involving the numbering/order of the FSM states.
e1f8c55
to
a4f1108
Compare
0864d34
to
e8323b2
Compare
I would like to add a test to define the behavior when there is no possible match in the vocabulary. Then it should be ready to merge. |
Now the |
171705d
to
85e3fbc
Compare
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.
@rlouf, I just pushed a change that makes the FSM start from the previous state it was in: i.e. it avoids decoding and rerunning the FSM for the entire sequence on each iteration. Tell me if that looks fine; otherwise, everything else looks good to me.
Agh, looks like the generated token sequences change shape when using |
7d0a35c
to
ec10981
Compare
# TODO: This check might be a little too strict, because I think that | ||
# while some states are made unreachable by a vocabulary (and will not | ||
# be present in the following set difference), there could still be | ||
# paths to terminal states emanating from the states that are reachable. | ||
states_with_transition = {x[1] for x in pstate_to_vocab.keys()} | ||
if len(self.regex_fsm.states.difference(states_with_transition)) > 0: | ||
raise ValueError( | ||
"The vocabulary does not allow us to build a sequence that matches the input regex" | ||
) |
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.
We should look into this. Perhaps as a follow-up issue.
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.
Good point. I opened #184 to track this.
outlines/text/masks.py
Outdated
mask = create_mask_from_regex(vocabulary, "^[0-9]+$") | ||
mask = create_mask_from_regex(vocabulary, r"(0|[+-]?[1-9][0-9]+?)") | ||
|
||
return mask | ||
|
||
|
||
def create_float_mask(vocabulary: Dict[str, int]) -> torch.BoolTensor: | ||
"""Create a mask to generate floating point numbers.""" | ||
mask = create_mask_from_regex(vocabulary, r"^(([0-9]+)?([.]([0-9]*)?)?|[.][0-9]+)$") | ||
mask = create_mask_from_regex( | ||
vocabulary, r"([+-]?((0|[1-9]+)([.][0-9]*)?)|([.][0-9]+))" | ||
) |
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.
The tests were getting a little flaky in CI, so I had to add these updates.
This allows one to add EOS transitions to the partial-parse-state-to-vocabulary maps produced by map_partial_states_to_vocab.
This refactoring also removed the need for the antecedent mapping option in `map_partial_states_to_vocab`.
ec10981
to
ad9d062
Compare
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.
OK, I think it's good to go now.
# Get the tokens we haven't already processed | ||
readable_tokens = token_seq[last_token_idx:] | ||
# excluding any EOS tokens | ||
not_eos_mask = [ |
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.
You should never get a sequence with an EOS
token here. Those are filtered out in Sequence.__call__
. Is it still worth keeping this check?
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.
If you start it out with a sequence like [[10, 2, 0, 0]]
, you would only want to process the first two. That's what it should be doing.
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.
When would that happen if you cannot get sequences with 0
by design?
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 believe I had to add it for the tests at one point, but that might not longer be true.
I only have one minor comment regarding |
This trivially generalizes #166 by defining a
Regex
class that can be initialized with any regex string. Theinteger
function now instantiatesRegex
with the appropriate regex string.Regex
classinteger
to initializeRegex
with a regex that only matches integersfloat
to initializeRegex
with a regex that only matches integersEOS
tokens for open-ended sequences