FiboaBaseConverter: bounded drops, and schemas up front - #264
ivorbosloper wants to merge 2 commits into
Conversation
8da77df to
5270f8a
Compare
ivorbosloper
changed the title
5270f8a to
2127c77
Compare
| AREA_KEY = "metrics:area" | ||
| # Properties that a schema requires to be non-null; rows lacking them cannot | ||
| # validate, so they are dropped (with a warning) rather than failing the run. | ||
| REQUIRED_NON_NULL = ("id", "crop:code") |
There was a problem hiding this comment.
This should be clear from the schemas and seems unnecessary to list in converters.
There was a problem hiding this comment.
You're right!
| area_calculate_missing = False | ||
| use_variant_as_determination = False | ||
| # rows lacking a REQUIRED_NON_NULL value are dropped up to this share, else it's an error | ||
| max_dropped_share = 0.01 |
There was a problem hiding this comment.
Pretty arbitrary number. I feel like we should decide for a consistent behavior (all or none). Maybe leave choice to users. Depending on the usecase you may want different behavior.
There was a problem hiding this comment.
This is some check that increases quality. Don't raise problems if the error margin is low, but set some default threshold.
There was a problem hiding this comment.
I feel like from a scientific perspective just ignoring errors when the error number is low is the wrong approach and doesn't necessarily increases quality. It depends on the usecase. We should probably discuss this further how we want to go ahead with such cases.
| load_file(uri) | ||
| break | ||
| except Exception as e: | ||
| if attempt == attempts - 1: |
There was a problem hiding this comment.
After a conversion retries make sense to me, but with prewarm I'd rather want the process to fail immediately instead of waiting 4mins, I think?
…emas first Two rules that every converter needs and none had. **Rows that cannot validate are dropped, up to 1% of the file.** A row with no `id` or no `crop:code` fails validation at the very end of a conversion, after everything has been read and written; the same goes for a row with an empty or missing geometry, which geopandas also refuses to Hilbert-sort. Dropping them silently would hide a broken mapping, so the share is bounded: above 1% the conversion fails and says which column and how many rows, because that is a converter bug rather than a few bad rows in the source. The lookup happens before columns are renamed, so the message names the source column a reader will find in the data. **Schemas are fetched before any real work.** The schema hosts (vecorel.org, fiboa.org) fail intermittently, and a blip after a long download used to kill the conversion at the write step, discarding hours of work. Eight attempts with exponential backoff, before the first byte of source data is read; `load_file` caches per process, so a successful pre-warm makes the write network-free. The Europe-LAND base converter comes along because the new rule exposes it: LT 2024 ships an entirely empty `crop_code` column beside a populated `crop_name`, so every row would be dropped. The name is the best available code there, and is used as one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVx9uQV2QPM8ecPAY3ZjXG
A converter should not have to repeat what its schemas already say: the core schema requires id and geometry, the crop extension crop:code and crop:code_list, and a converter that declares an extension takes on its rules with it. The hardcoded pair is gone; the merged schema of the collection says what every row must carry, out of the copy _prewarm_schemas has already fetched. And the pre-warm itself fails fast now. It runs before anything is downloaded or converted, so waiting four minutes on a schema host buys nothing — one retry for a dropped connection, then out. The retries were written when the fetch happened at the write step, with hours of work behind it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVx9uQV2QPM8ecPAY3ZjXG
008f70e to
b5a5921
Compare
|
vecorel-cli 0.2.18 carries this upstream. Its #280 bumps the pin and brings along the two converter fixes the checks fire on (hr's rolling |
2 participants
First of the structural PRs (datasets: #231-#263). Two rules every converter needs and none had.
Rows that cannot validate are dropped, up to 1% of the file. A row with no
idorcrop:code, or an empty geometry, fails at the very end of a conversion — hours of work discarded for a handful of rows. Dropping silently would hide a broken mapping, so above 1% the conversion fails and names the source column and the count. This is how si 2019 was caught publishing 820,151 fields with no id (#231).What "required" means comes from the schemas, not from a list here. The core schema requires
idandgeometry, the crop extensioncrop:codeandcrop:code_list, and a converter that declares an extension takes on its rules with it — so there is nothing for a converter to repeat. (Thanks @m-mohr; the first version hardcoded("id", "crop:code").)Schemas are fetched before any source data is read, so a converter knows what every row must carry before it downloads anything — and a schema host that is down says so immediately rather than after a wait. One retry for a dropped connection, then out: the long backoff made sense when the fetch happened at the write step, with hours of work behind it.
One converter comes along: Europe-LAND's LT 2024 ships an empty
crop_codebeside a populatedcrop_name, so every row would be dropped; the name is the best code there is.max_dropped_sharestays a class attribute, so a converter whose source really is that patchy can raise it, and the error says how many rows it would have dropped.#265 builds on this — same file.