search

GSTT-CSC / MLOps

Framework for building ML apps
GNU General Public License v3.0
9 stars 5 forks source link

Add citation file to develop branch #40

Closed alexandertdeng closed 2 years ago

alexandertdeng commented 2 years ago

Format: First Author, alphabetical middle authors, Last Author

hshuaib90 commented 2 years ago

@laurencejackson - do you know why these tests are failing now?

laurencejackson commented 2 years ago

Hmm, not sure, it looks like the MLOps server isn't starting - specifically the minio part. I will look into it.

laurencejackson commented 2 years ago

@hshuaib90 and @alexandertdeng, I think the reason the Tests/build action is failing is that the environment variables used to create the mlflow server in the test environment are current repository secrets. This PR is for a forked repo (alexandertdeng:add-citation-file) into the develop branch of this repo (GSTT-CSC:develop), since they are separate repositories the secrets are not shared with the fork.

Options to resolve:

  1. Alex sets up his own secret in the forked repo
  2. we share the secrets with forked repos (https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/)

I think option 2 is the best, especially since the 'secrets' are really just randomly made up user accounts and passwords, which don't actually reflect the deployed server.

hshuaib90 commented 2 years ago

Yes I agree that option 2 is best. Are you ok to set that up?

laurencejackson commented 2 years ago

OK sorry, it looks like GitHub have removed (or maybe never implemented?) that feature. I'd like to suggest option 3

  1. We make the "secret variables" plain text but meaningless random strings.

@alexandertdeng could you change lines 19-24 in .github/workflows/python-app.yml so that it reads:

MYSQL_DATABASE: mlflow
MYSQL_USER: msdndfvk
MYSQL_PASSWORD: adfg3245
MYSQL_ROOT_PASSWORD: sfdghj43256!!
MINIO_ROOT_USER: fdash
MINIO_ROOT_PASSWORD: rstjDFH345!

and push these changes as you have previously so that it updates this PR.

alexandertdeng commented 2 years ago

@laurencejackson This pull request should now be updated with the plaintext strings

laurencejackson commented 2 years ago

Thanks @alexandertdeng, the tests are still failing, but I think this is because we redefine the MINIO_ROOT_USER and MINIO_ROOT_PASSWORD variables in the test function. This was originally done so that tests could be run locally.

For now could you delete the lines that redefine these env variables?

I think this should be in tests/mlops/test_Experiment.py lines 13 and 14 in setup(), the lines to remove are 

        os.environ['MINIO_ROOT_USER'] = 'minioadmin'
        os.environ['MINIO_ROOT_PASSWORD'] = 'minioadmin'

could you also append the below to the end of the scrambled environment variables you added to .github/workflows/python-app.yml. Sorry, I forgot to include this one before.

MLFLOW_S3_ENDPOINT_URL: http://s3:9002

alexandertdeng commented 2 years ago

@laurencejackson This pull request should now be updated with the changes you specified

laurencejackson commented 2 years ago

Thanks Alex, the build is now hanging during the pytest step. I've cancelled the run for now to save build minutes and will investigate the cause.

laurencejackson commented 2 years ago

Sorry it took so long to get back to you @alexandertdeng, the builds are failing because your fork does not have permission to write to the storage where we store the "tests passing" and "test coverage" badges that appear on the readme! So obviously nothing to do with your code, which looks great so I'll go ahead and merge. Thanks for the contribution!