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 test and build pipelines #87

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

DhanshreeA
Copy link
Member

@DhanshreeA DhanshreeA commented Jan 8, 2025

Refer to parent issue: ersilia-os/ersilia#1484

This PR introduces significant refactoring to testing and building workflows for all the models within the hub.

Please refer to the attached diagram for an overview of the workflows and the sequence in which they will be triggered.
<TODO: Insert schema here>

Major changes in the PR:

  • Updated the test-model-pr.yml workflow to use ersilia test command to test a model repository.
  • Updated the test-model.yml workflow to use ersilia test command to test a model repository.
  • Refactored the Docker build workflows to build and push both AMD and ARM images to be consumed in a test in the next step. These images are tagged dev when built. Since ersilia now supports fetching models with specific tags, we can leverage that to fetch dev images.
  • Added a workflow to run after Docker build to test the built image on both AMD and ARM platforms using the testing playground from Ersilia. This workflow automatically tags the image if it worked on at least one platform with the date of the workflow run as well as the default latest tag. This workflow can be found in the test-model-image.yml.

Minor changes in the PR:

  • Code clean up, particularly in the upload-model-to-s3.yml workflow.
  • Renamed workflows for better clarity.

Further improvements:

  1. Currently the test-model-pr.yml workflow runs ersilia CLI in a Python 3.10 environment. Technically, ersilia CLI is supported for Python versions 3.8 to 3.12, therefore, regardless of which Python version the model requires, the CLI should be run for the model in all the aforementioned Python versions.

@DhanshreeA DhanshreeA self-assigned this Jan 8, 2025
@DhanshreeA DhanshreeA marked this pull request as draft January 8, 2025 15:08
@DhanshreeA DhanshreeA force-pushed the refactor-test-and-build-pipelines branch 2 times, most recently from 37a76f9 to 9c22e71 Compare January 8, 2025 16:52
@DhanshreeA DhanshreeA force-pushed the refactor-test-and-build-pipelines branch from 9c22e71 to fe3dfb6 Compare January 8, 2025 16:53
@GemmaTuron
Copy link
Member

GemmaTuron commented Jan 8, 2025

Thanks @DhanshreeA for the summary, I have a few questions:
A general one is, when exactly does the test module fail and when it gives a warning? Could it be that things are not entirely correct but the workflow is shown as passed?

For the specific workflows:

  • test-model-pr.yml: tests the model once a PR is opened right? so the inspect command will fail for some fields (like DockerHub URL). Should the --inspect flag be used?
  • test-model.yml: I do not find this file in the repository, please can you double check? Did you rename it to model-source?
  • test-model-source.yml: when is this triggered? I see redundancies:
  1. The model is fetched, served and run, but that is also done by the test command
  2. The model output is evaluated for nulls, when actually the test command also checks for it.
  3. I am unsure of what is the status of the example generation. Will all models have an example file inside? How many molecules will be available there?
  • test-model-image.yml: you are aiming to use the playground to test in Linux and Mac, but it only works in Linux.

We also need to decide what we do with the Python versions. Testing everything every time with each python version is simply going to be too time consuming.

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