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

OMPP: Moved copy file directives from start-custom.sh to start-oms.sh #553

Merged
merged 17 commits into from
Nov 29, 2023

Conversation

jacek-dudek
Copy link
Contributor

Description

What your PR adds/fixes/removes

Tests / Quality Checks

Are there breaking changes?

Ask yourself the next question;

  • Do we want to maintain the previous image from which we had to do breaking changes from?

If no, then carry on. If yes, there is a breaking change and we want to maintain the previous image do the following

  • Create a new branch for the current version (ex v1) based off the current master/main branch
  • Increment the tag in the CI for pushes to master/main (v1 to v2)
  • Change the CI that on pushes to the newly created "v1" branch (the name of the newly created branch we want to maintain is) it will push to the ACR.

Automated Testing/build and deployment

  • Does the image pass CI successfully (build, pass vulnerability scan, and pass automated test suite)?
  • If new features are added (new image, new binary, etc), have new automated tests been added to cover these?
  • If new features are added that require in-cluster testing (e.g. a new feature that needs to interact with kubernetes), have you added the auto-deploy tag to the PR before pushing in order to build and push the image to ACR so you can test it in cluster as a custom image?

JupyterLab extensions

  • Are all extensions "enabled" (jupyter labextension list from inside the notebook)?

VS Code tests

  • Does VS Code open?
  • Can you install extensions?

Code review

  • Have you added the auto-deploy tag to your PR before your most recent push to this repo? This causes CI to build the image and push to our ACR, letting reviewers access the built image without having to create it themselves
  • Have you chosen a reviewer, attached them as a reviewer to this PR, and messaged them with the SHA-pinned image name for the final image to test on the dev cluster (e.g. k8scc01covidacrdev.azurecr.io/jupyterlab-cpu:746d058e2f37e004da5ca483d121bfb9e0545f2b)?

@Souheil-Yazji Souheil-Yazji changed the title Moved copy file directives from start-custom.sh to start-oms.sh OMPP: Moved copy file directives from start-custom.sh to start-oms.sh Nov 17, 2023
@Souheil-Yazji Souheil-Yazji added the auto-deploy Trigger manual CI steps for this PR label Nov 17, 2023
Copy link
Contributor

@Souheil-Yazji Souheil-Yazji left a comment

Choose a reason for hiding this comment

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

Approved with nitpicks to be cleaned up.

resources/common/start-oms.sh Outdated Show resolved Hide resolved
resources/common/start-oms.sh Show resolved Hide resolved
resources/common/start-oms.sh Outdated Show resolved Hide resolved
resources/common/start-oms.sh Outdated Show resolved Hide resolved
resources/common/start-oms.sh Outdated Show resolved Hide resolved
@Souheil-Yazji
Copy link
Contributor

@jacek-dudek 1 final question, will this change the workflow of any users currently using OpenMPP? If yes, do we need to notify them of the change.

@jacek-dudek
Copy link
Contributor Author

Let me spin up a notebook server with the container sourced from the master branch and do a quick side-by-side comparison of the user experience. I will post a follow-up here in a bit.

@jacek-dudek
Copy link
Contributor Author

I encountered an issue with the openmpp-13 version of the web service, where some sequence of job requests starts throwing up the "Server offline or model failed to start" error. I'm going to try to reproduce and figure out if/how this is related to our changes. Please hold off on the review approval for a bit as I investigate this.

jacekzbigniewdudek added 4 commits November 22, 2023 18:42
…vice. This should make the web service write to its own log file. Should help me diagnose the hung server errors that I was getting in the UI.
@KrisWilliamson KrisWilliamson self-requested a review November 23, 2023 19:08
Copy link
Contributor

@KrisWilliamson KrisWilliamson left a comment

Choose a reason for hiding this comment

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

The old mpi template has to be removed from the project. It causes an error is it is used with AAW.

@Souheil-Yazji Souheil-Yazji merged commit 8f7487e into master Nov 29, 2023
7 checks passed
@Souheil-Yazji Souheil-Yazji deleted the openmpp-13 branch November 29, 2023 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-deploy Trigger manual CI steps for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verify that an MPIJob resource is created when current solution gets run.
3 participants