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.

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

Release single-step evaluation framework and wrappers for several model types #14

Merged
merged 13 commits into from
Jul 14, 2023

Conversation

kmaziarz
Copy link
Contributor

This (fairly large) PR releases the majority of the single-step evaluation code, including the eval script itself, inference wrappers for seven model types, and glue code to plug these models into search. The changes are split into semantically meaningful commits. Some notes for reviewing:

  • Wherever the commit message starts with "Release", this means our internal code was copied verbatim (or almost verbatim), and thus at the very least it went through code review in the past.
  • Some of the commits edit the configuration around CI and pre-commit. These changes were needed partly because the model wrappers contain dependencies which are not installed in our default environment, and thus one needs to tell mypy to ignore external libraries it doesn't know about.
  • Some bits of internal code are not part of this release; the "Make the evaluation pipeline easy to extend" commit makes it easier to plug them in into syntheseus from outside.

A few notable things which are still missing, to be added in future PRs to avoid making this one even larger:

  • Detailed setup instructions for each model type (e.g. environment definitions).
  • Model checkpoints for each model type.
  • More tests covering syntheseus/reaction_prediction (currently the coverage is quite low, partly because the wrappers themselves cannot be tested easily without setting up dedicated environments for each, but there are also places which are just not adequately covered by tests yet).

Copy link
Collaborator

@AustinT AustinT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few small comments, but overall looks good to me. The tests all pass locally for me.

I was not able to run eval.py locally because I don't have the correct model files or example configs, so I can't independently confirm that it works. By extension, I could not test the "Make the evaluation pipeline easy to extend" commit. Did you test it locally @kmaziarz ? If so I am happy to merge it, and provide documentation / configs / model weights in a future PR.

.pre-commit-config.yaml Show resolved Hide resolved
syntheseus/reaction_prediction/chem/utils.py Show resolved Hide resolved
syntheseus/reaction_prediction/utils/misc.py Outdated Show resolved Hide resolved
syntheseus/reaction_prediction/utils/misc.py Outdated Show resolved Hide resolved
syntheseus/tests/reaction_prediction/cli/test_eval.py Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@kmaziarz
Copy link
Contributor Author

I was not able to run eval.py locally because I don't have the correct model files or example configs, so I can't independently confirm that it works. By extension, I could not test the "Make the evaluation pipeline easy to extend" commit. Did you test it locally @kmaziarz?

Yes, I re-ran the single-step evaluation for all seven model types, and confirmed that all resulting numbers match the table in the paper 🙂

@kmaziarz kmaziarz force-pushed the kmaziarz/release-reaction-prediction branch from 11c81ce to c06669c Compare July 13, 2023 14:51
@kmaziarz kmaziarz merged commit 9de0f47 into main Jul 14, 2023
@kmaziarz kmaziarz deleted the kmaziarz/release-reaction-prediction branch July 14, 2023 10:57
kmaziarz added a commit that referenced this pull request Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants