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

refactor to be usable and testable without modal #8

Merged
merged 28 commits into from
Aug 20, 2024

Conversation

kjappelbaum
Copy link
Collaborator

@kjappelbaum kjappelbaum commented Aug 10, 2024

This is a major change to the codebase. In some parts, it leads to more complications, but in other parts, it makes things a lot easier.

It would be great if the tools/functions here could be used and tested independently from the modal (to increase the potential for reusability—also for some of our projects). Hence, I refactored the project into an installable Python package.

I'd now define all the tools as Python functions that can be used and tested as conventional Python functions. In addition, they will now be imported into the modal.py module with the relevant dependencies.
Right now, the modal tests still test some functionality. However, they likely should be refactored to only test if the call/the promised signature works.

A great advantage of modal is that we do not need to worry about dependency conflicts. However, we can still have that if needed.

For clarity, I also started renaming all methods get_x. We might find a better convention later, but I wanted to unify what we have.

I also have the RXN lookup (#6) built as FAISS DB (https://kjappelbaum--rxn-search-webapp-search.modal.run/doc), but I'll leave this for a separate PR once this here is merged as it will be too messy otherwise

This was linked to issues Aug 11, 2024
@kjappelbaum
Copy link
Collaborator Author

The modal tests work for me locally, perhaps there is some issue with the secret on GitHub.

@kjappelbaum kjappelbaum marked this pull request as ready for review August 11, 2024 15:14
@kjappelbaum kjappelbaum marked this pull request as draft August 11, 2024 15:14
@kjappelbaum
Copy link
Collaborator Author

Oh ... I forgot that secrets in PRs are more complex https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

@kjappelbaum
Copy link
Collaborator Author

kjappelbaum commented Aug 11, 2024

ok - I should have read the docs more carefully. The pull_request_target action is only triggered for internal PRs and not for PRs from forks.

I do not see a good way to provide secrets to actions on PRs from forks. Also the dispatch does not run on PRs.

In this way, the refactoring at least gives us some testability.

@kjappelbaum kjappelbaum marked this pull request as ready for review August 11, 2024 18:05
Copy link
Contributor

@whitead whitead left a comment

Choose a reason for hiding this comment

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

I agree with this idea - thanks for taking a crack at it!

- name: Deploy to dev
run: modal deploy tools/deploy.py
run: poetry run chemenv deploy
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we try to use uv instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Optional - just we've found it to speed up build times quite a bit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, I'm also moving ChemBench to uv atm

@kjappelbaum
Copy link
Collaborator Author

Thanks for giving it a look, @whitead!

@kjappelbaum kjappelbaum merged commit cad4a85 into ur-whitelab:main Aug 20, 2024
2 checks passed
@kjappelbaum kjappelbaum deleted the refactor branch August 20, 2024 04:06
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.

implement periodic table endpoint implement 1H NMR endpoint
2 participants