Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve handling of unsupported Matcher attributes #4070

Closed
adrianeboyd opened this issue Aug 2, 2019 · 2 comments
Closed

Improve handling of unsupported Matcher attributes #4070

adrianeboyd opened this issue Aug 2, 2019 · 2 comments
Labels
enhancement Feature requests and improvements feat / matcher Feature: Token, phrase and dependency matcher feat / ux Feature: User experience, error messages etc.

Comments

@adrianeboyd
Copy link
Contributor

Feature description

I decided to separate this from #4069 (see also #4063 ) since it's not quite the same issue.

With Matcher, it's very confusing that "bad" parts of patterns (e.g., with attributes that aren't supported) are silently discarded and end up matching every token rather than no token.

{'ASDF': True} shouldn't be equivalent to {}.

How slow is validation? Would it make sense to make validate=True the default? Are there simpler checks that could be added for when attributes are discarded that don't require full validation so that you could provide warnings or errors in these cases?

I think that less confusing default behavior would be:

  • {'ASDF': True} matches nothing
  • there are always warnings or errors when it is known that patterns will match nothing due to this kind of problem
@ines ines added enhancement Feature requests and improvements feat / matcher Feature: Token, phrase and dependency matcher labels Aug 2, 2019
@ines
Copy link
Member

ines commented Aug 2, 2019

Yes, I agree! 👍

How slow is validation? Would it make sense to make validate=True the default? Are there simpler checks that could be added for when attributes are discarded that don't require full validation so that you could provide warnings or errors in these cases?

Yes, we should probably just consider implementing simpler checks for just the key names and whether they map to a valid attribute or not.

The current validation uses jsonschema, which we've made an optional dependency (we didn't want it to drag in too many other dependencies down the tree). JSON schema validation is nice because it lets you write pretty complex constraints and gives you helpful and detailed error messages. It's also cross-Python compatible. (Once we drop 2.7 and 3.5, we could probably implement something similar using native Python type hints and make that enabled by default.)

@ines ines added the help wanted Contributions welcome! label Aug 2, 2019
adrianeboyd added a commit to adrianeboyd/spaCy that referenced this issue Aug 12, 2019
@adrianeboyd
Improve token pattern checking without validation
823fdc8 
Add more detailed token pattern checks without full JSON pattern validation and
provide more detailed error messages.

Addresses explosion#4070 (also related: explosion#4063, explosion#4100).

* Check whether top-level attributes in patterns and attr for PhraseMatcher are
  in token pattern schema

* Check whether attribute value types are supported in general (as opposed to
  per attribute with full validation)

* Report various internal error types (OverflowError, AttributeError, KeyError)
  as ValueError with standard error messages

* Check for tagger/parser in PhraseMatcher pipeline for attributes TAG, POS,
  LEMMA, and DEP

* Add error messages with relevant details on how to use validate=True or nlp()
  instead of nlp.make_doc()

* Support attr=TEXT for PhraseMatcher

* Add NORM to schema

* Expand tests for pattern validation, Matcher, PhraseMatcher, and EntityRuler
@adrianeboyd adrianeboyd mentioned this issue Aug 12, 2019
Merged
3 tasks
@ines ines added feat / ux Feature: User experience, error messages etc. and removed help wanted Contributions welcome! labels Aug 15, 2019
ines added a commit that referenced this issue Aug 21, 2019
@adrianeboyd @ines @honnibal
Improve token pattern checking without validation (#4105)
8fe7bdd 
* Fix typo in rule-based matching docs

* Improve token pattern checking without validation

Add more detailed token pattern checks without full JSON pattern validation and
provide more detailed error messages.

Addresses #4070 (also related: #4063, #4100).

* Check whether top-level attributes in patterns and attr for PhraseMatcher are
  in token pattern schema

* Check whether attribute value types are supported in general (as opposed to
  per attribute with full validation)

* Report various internal error types (OverflowError, AttributeError, KeyError)
  as ValueError with standard error messages

* Check for tagger/parser in PhraseMatcher pipeline for attributes TAG, POS,
  LEMMA, and DEP

* Add error messages with relevant details on how to use validate=True or nlp()
  instead of nlp.make_doc()

* Support attr=TEXT for PhraseMatcher

* Add NORM to schema

* Expand tests for pattern validation, Matcher, PhraseMatcher, and EntityRuler

* Remove unnecessary .keys()

* Rephrase error messages

* Add another type check to Matcher

Add another type check to Matcher for more understandable error messages
in some rare cases.

* Support phrase_matcher_attr=TEXT for EntityRuler

* Don't use spacy.errors in examples and bin scripts

* Fix error code

* Auto-format

Also try get Azure pipelines to finally start a build :(

* Update errors.py


Co-authored-by: Ines Montani <ines@ines.io>
Co-authored-by: Matthew Honnibal <honnibal+gh@gmail.com>
@ines ines closed this as completed Aug 21, 2019
@lock
Copy link

lock bot commented Sep 20, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
enhancement Feature requests and improvements feat / matcher Feature: Token, phrase and dependency matcher feat / ux Feature: User experience, error messages etc.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@adrianeboyd @ines