Remove Rxn-INSIGHT reaction classification - #978
Merged
Conversation
ord_schema.search.execute imports ord_schema.artifacts.pivot, which imports inflection to name a pivot's ordinal columns; inflection was declared only by the orm extra. A development checkout installs every group, so the suite passed while `pip install ord-schema[search]` produced a subpackage that raises ModuleNotFoundError on import. The test walks the import graph from the search entry points and asks importlib.metadata which distribution provides each module reached, which needs no hand-maintained map of module to distribution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each profile owns the modules no more specific one claims, so the base install is checked against everything the extras do not carry, and an extra is checked against what its own subpackage imports. An import inside a try is one the module states it can do without, so it belongs to the profile owning the guarded module rather than to this one. Doing that to the ORM turned up a second gap: reaction_class.py imports rxn-insight and rxnmapper, and database.py tells whoever hits the resulting ImportError to install ord-schema[reaction-class], which pyproject did not define -- so following the instruction installed nothing and changed nothing. The extra exists now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Classification ran inside the ORM's derived stage, behind an opt-in flag and a guarded import, and the labels it wrote went to derived.reaction_classes. That is not the shape this should take: the model is a transformer per worker, and the work belongs to a job that can be given a GPU rather than to the ingest that loads a dataset. Removed the module, the classify pass and its shard pool, the flags that reached them, the mapper for the table, and the pages that documented them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> # Conflicts: # ord_schema/dependencies_test.py
Merging main brought the reaction-class extra back: main gained it in a squash-merge, so relative to the merge base only main had touched those lines and the deletion did not survive. Removed again, with the lockfile regenerated. The rest is prose that says "reaction classes" in words rather than in the identifiers the first pass grepped for -- the schema description and the loading overview in the ORM README, the same sentence in the orm-database-load skill, the loading module docstring, and two lines in database.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reaction classification ran inside the ORM's derived stage, behind
--classify_reactionsand a guarded import, writing labels toderived.reaction_classes. Removing it: the model is a transformer loaded per worker, and that work belongs to a job that can be given a GPU rather than to the ingest that loads a dataset.Changes
ord_schema/orm/reaction_class.pyand its test.database.py: the guarded import,_classify_reactions,classify_dataset, and theclassify_reactionsargument toupdate_derived_data.loading.py: the classify pass,_classify_shard,_CLASSIFY_JOBS_CAP, and theclassify_reactions/classify_jobsarguments toload_datasetsandderive_dataset.scripts/add_datasets.py:--classify_reactionsand--classify_jobs.derived_mappers.py: theReactionClassesmapper, which is the schema change — see Notes.pyproject.toml: thereaction-classextra, added one PR down the stack and now unnecessary.Testing
uv run pytest -n auto— 1186 passed. All three of the suite's skips are gone with it, since every one was a test the missing extra was skipping:No consumer at origin references
reaction_class,classify_dataset,classify_reactions, orreaction_classes— checked againstorigin/mainof ord-interface, ord-app, ord-infrastructure, and ord-data.Notes
This changes the schema. The mappers are what create the tables, so a database built after this has no
derived.reaction_classes. An existing one keeps the table and its rows, now unmapped and untouched by any pass here; dropping it is a separate decision, and a GPU job that writes classifications later can define its own.Stacked on #976, which is where the
reaction-classextra was added — it was a true statement about the tree as it stood, and this PR removes both the extra and what needed it. Retarget to main once #976 lands.🤖 Generated with Claude Code
Greptile Summary
The PR removes Rxn-INSIGHT reaction classification from the ORM and its installation surface.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Reviews (2): Last reviewed commit: "Merge main into agent/remove-reaction-cl..." | Re-trigger Greptile