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

GitHub Actions #9

Closed
cc-a opened this issue Apr 2, 2020 · 4 comments · Fixed by #24
Closed

GitHub Actions #9

cc-a opened this issue Apr 2, 2020 · 4 comments · Fixed by #24

Comments

@cc-a
Copy link
Contributor

cc-a commented Apr 2, 2020

Following on from #4 I wanted discuss a little what should be achievable using GitHub actions to support publishing the model. At the present time I see GitHub actions as providing potentially two useful features.

  1. Testing that the model runs on a variety of different operating systems (Linux + Windows), under different environments (Docker + Conda). This is basic validation that other people can use the model in a way that is fairly agnostic w.r.t how it is setup and run. This catches other simple to make errors such as adding a new dependency but forgetting to add it to environment.yml or forgetting to check in a new file with a commit.

The reason I say runs above is to try and emphasise the limitations of this form of testing. This will only check that the model can be run. It will provide no assurance about the correctness of the code or that any results produced are meaningful. It can be easy to rely too much on tests of this type but they should be considered only the most basic and crude check of a set of changes. With the attention this project has garnered you might find random PRs coming in (including from me apparently) and I don't want to give you a false sense of security about merging them. In an ideal world the project would include test code that could more robustly check incoming changes but that will have remain a manual process for now.

  1. Automate the building of the Docker image and its publication for new commits to master. This saves users from having to build the Docker image themselves but also makes the model much easier to run with Docker alternatives such as Singularity that are compatible with HPC environments.

I don't think either of these is very difficult to put together. I'm sure there are all sorts of things that GitHub actions could be used for here that I've not thought so let me know if any other automatible tasks spring to mind!

Please let me know what you think. If you're interested I think a sensible way forward is for me to prototype something in my fork so you can see it in action.

@harrisonzhu508
Copy link
Collaborator

Hi Chris, thank you so much for your detailed suggestions.

I agree that point 1 is useful, it's always good to simply check that we can run the code!

You are absolutely right, we haven't done any unit testing yet, so that is something we should look at in the future. I think the idea of having Docker is itself very exciting, so thanks for helping us today!

I think point 2 is very useful as well, as it also promotes the open source nature of our work.

If it wouldn't take up too much of your time, it would be very beneficial for us and everyone who is interested in this work if you could have a look at these 2 features in your fork, and submit a pull request. Do let us know if you need any clarification or help at any point! Let me know and I'll close my previous PR :)

Thanks,
Harrison

@s-mishra
Copy link
Member

s-mishra commented Apr 2, 2020

Thanks Chris, all looking forward to two additions you mentioned. They would be very useful

@cc-a cc-a mentioned this issue Apr 4, 2020
@cc-a
Copy link
Contributor Author

cc-a commented Apr 4, 2020

If you point me towards the Dockerhub repository, I'll put a quick PR together updating the instructions for running with Docker.

@harrisonzhu508
Copy link
Collaborator

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 a pull request may close this issue.

3 participants