Adopt external data parsing/normalisation lib #1368
Conversation
followthemoney/types/identifier.py
Outdated
| def clean_text( | ||
| self, | ||
| text: str, | ||
| fuzzy: bool = False, | ||
| format: Optional[str] = None, | ||
| proxy: Optional["EntityProxy"] = None, | ||
| ) -> Optional[str]: | ||
| if format is not None and format in FORMATS: | ||
| type_ = FORMATS[format] | ||
| return type_.normalize(text) | ||
| return text |
There was a problem hiding this comment.
-
Just to sanity check my understanding of FtM: Validating concrete identifier formats is a new feature, right? Actually, if I don’t miss anything, the only use for property type formats so far was the date type, right?
-
I vaguely remember that we talked about adding the identifier format to the schema, so e.g.
Thing:wikidataIdwould be anidentifierin thewikidataformat (and when validating an entity, values of this property would need to be a valid Wikidata QID)? Is that still something you’d like to add? I think it’s a good idea, but in that case would like to check on our side if/where these stricter validations could cause problems (e.g. in scrapers).
There was a problem hiding this comment.
Nice catch! So this is new functionality, but for the moment it would only come into effect in a mapping (that's the only place the format is set from) which would specify format for an identifier field. So it's completely opt-in.
The other option - making properties themselves know format - makes a ton of sense semantically (they're literally called wikidataId, swiftBic, imoCode, innCode, etc.) but it would create a breaking change in the sense that old mappings might produce much less data because all of a sudden a whole ton of INNs are recognised as invalid. There's maybe an argument for that to be a good thing, but it'd definitely be a surprising thing.
Maybe there's even a way of half-doing it: some props, like wikidataId have strong expectations, and we could introduce it there. By doing that we could also make IBAN fields from type: iban into type: identifier, format: iban which makes a bit more sense.
WDYT?
There was a problem hiding this comment.
but it would create a breaking change in the sense that old mappings might produce much less data because all of a sudden a whole ton of INNs are recognised as invalid. There's maybe an argument for that to be a good thing, but it'd definitely be a surprising thing.
Personally, I’d say that’s a good thing as long as there’s maybe some logging in the mapping logic when values are removed because they are invalid, allowing the monitoring of e.g. Memorious scrapers for validation issues.
But I’ve also forwarded this thread to our data team because they’d be much more qualified to assess the potential impact on our scraper fleet.
There was a problem hiding this comment.
The other use case where validation is relevant from Aleph’s perspective is OCR/pattern extraction: When extracting IBANs from scanned documents, there might be OCR issues that e.g. cause the extracted IBAN to not match validation. While such an invalid IBAN might not be as useful for cross referencing, there might still be value in storing and exposing the extracted IBAN to users.
This isn’t really relevant to this PR, as the PR doesn’t change how IBANs are validated (there’s just an additional layer of abstraction), but something to keep in mind for the future and possibly when adding other identifier formats. (For most of the other identifier formats implemented in rigour, it’s probably much more likely that they are extracted from company registries etc. rather than from a scanned document, but I might be incorrect here.)
setup.py
Outdated
| @@ -35,6 +35,7 @@ | |||
| "types-PyYAML", | |||
| "sqlalchemy2-stubs", | |||
| "banal >= 1.0.6, < 1.1.0", | |||
| "rigour >= 0.3.0, < 1.0.0", | |||
There was a problem hiding this comment.
Maybe that’s a dumb question, but what’s the advantage of extracting this functionality into a separate package compared to a separate module in followthemoney? As it contains validation/normalization logic for FtM data, I guess most projects that would make use of this (inlcuding Aleph, nomenklatura, …) would require followthemoney anyways?
There was a problem hiding this comment.
That's a fair point, it could live in followthemoney. This is more meant to address a social issue than a technical one: at present, I'm treating FtM releases as expensive because they involve "y'all" - so I try to only push out a release here if it is super relevant.
The problem I have is that this leads to a lot of "plaque" in our upstream libs - for example when it comes to extra name and identifier processing code like here: https://github.com/opensanctions/nomenklatura/blob/main/nomenklatura/util.py#L95-L206
The idea with rigour is to combine all the intel about the types and how to process them into one place, and to basically make a promise that the upstream compatibility with FtM will be preserved, while not putting a limitation on the amount of e.g. name processing stuff that we can add as needed.
There was a problem hiding this comment.
Thanks for clarifying, that makes sense!
followthemoney/types/identifier.py
Outdated
| @@ -1,9 +1,14 @@ | |||
| import re | |||
| from typing import Optional, TYPE_CHECKING | |||
| from rigour.ids import FORMATS | |||
There was a problem hiding this comment.
Maybe these could be exposed as a class attribute on the IdentifierType class? Then the available formats (and even their docstrings) could be included in the JSON dump of the model, making it possible to include them in the documentation and to pick them up in the TS lib (maybe not to do full validation on the client side, but at least to render some format hints or something like that.)
There was a problem hiding this comment.
I love that idea, importing the raw dict felt wrong. Will mint a new rigour release later :)
|
@tillprochaska I've done another pass at this, implementing the helper methods for fetching a format, and also adding a One thing I've skipped so far is the annotations for |
@pudo Nice! I’m off until Tuesday next week, but will take a closer look once I’m back.
One thing I didn’t really think about so far with regards to Aleph is that we also use the types to merge unique values for all properties of the same type (all names, all IBANs, …) in the backend (indexing entities and computing statistics) and in the frontend (when displaying lists of entities). We could probably work around that by using the new But maybe IBANs are (ignoring the implementation details) a sufficiently independent concept compared to other ID formats that this warrants IBANs being a separate type? Similar to how IP addresses or phone numbers could probably also be considered identifiers that follow a specific format, but they are still separate types. Haven’t made up my mind about that, and probably I’m overthinking it. |
|
@tillprochaska - This should be in a place right now where it's safe to merge; and rigour has seen a bit of action on our end to make sure it's not totally messed up. Would appreciate your feedback! |
|
@pudo Hey, sorry for the late reply. I got confirmation from @brrttwrks that he’s fine with the stricter validation for The only other request from our data team was some logging when validation fails (and values are cleaned from entities) before we implement stricter validation. And possibly the option to disable validation/cleaning on a per-property level in mappings. I’ll try to open a PR that does that today or tomorrow. Thanks also for restoring IBANs as a separate type. We rely on that in quite a few places in Aleph. I’ll also take another look at the code today. |
|
No this is definitely an open heart operation, thanks for taking the time to work through it! |
We talked about this briefly during the Berlin meeting - the idea here it to make rigour into a more in-depth data normalisation library that can also hold a lot of the fundamental handling code for human names that's currently in
nomenklatura.