microsoft / hi-ml Goto Github PK

Sketch basic unit tests & mocks for:

1. Uploading a single Python file to AzureML, running it, and returning the run_id. Then wait for run completion, then downloading output files and stdout

2. Uploading a single Python file and a data file, e.g. data.csv, rest as above.

3. Uploading two Python files, to check imports, rest as above.

Building a docker image costs about 20min, that's too long for a PR build.

• In the PR builds, only execute normal conda runs without docker.
• In the daily build, test docker image generation, and in particular test if the docker_shm_size is respected
• Ensure that, if the build fails, people get an email that is not the standard github email

Should not cause merge conflicts
Individual files that are collated into a changelog file

• run_config has a field max_run_duration_seconds, make that an argument of submit_if_needed
• Add an argument for raise_on_error: wait_for_completion(self, ... raise_on_error=True):
• Add an argument for returning after submitting the job, rather than exiting
• Add an argument for experiment name

We have sporadic test pipeline failures when building a Docker image in the AzureML jobs. Example here The job claims that package version post282 does not exist - but the github build agents successfully pulled that version a few minutes before. This probably comes from a package mirror that AML are using, that is not completely up to date with test.pypi.
As a workaround, we could publish to an MS internal package feed. AML would probably not have a cache of that, and hence always use the latest.

PR #36 has some unanswerd comments around naming, mocking, documentation, and repitition here.

Helps to delimit where the reordering of the PIP indexes applies.

• Script that starts tensorboard server on local box
• should pick up most recent run automatically, or accept a run_id argument
• See Word document for a link to examples from the chestXray project
• This script should be installed automatically in /bin when installing the package.
• Check with Mel if there are any extra gadgets that would be helpful here. For example: Should we not only store most_recent_run.txt, but a full submission history (most_recent_runs.txt), and tensorboard monitoring can then pick up the last 3, for example?

• Create a new AzureML workspace and storage account only for the hi-ml package. Ideally, this should be tried out with Azure CLI tools - it would be ideal if we had a completely script-driven way of creating
  • resource group
  • workspace
  • storage account
  • service principal
  • data store
  • compute cluster
• Document all the necessary steps in an MD file, store the script(s) to create everything
• Switch the github workflow to this new workspace and service principal

Potential omission from #55 where it says

For CodeQL, please ensure the following (detailed instructions for CodeQL can be found here):
Select the source code language in the CodeQL task.
If your application was developed using multiple languages, add multiple CodeQL tasks.
Define the build variable LGTM.UploadSnapshot=true.
Configure the build to allow scripts to access OAuth token.
If the code is hosted in Github, create Azure DevOps PAT token with code read scope for dev.azure.com/Microsoft (or ‘all’)
organization and set the local task variable System_AccessToken with it. (Note: This only works for YAML-based pipelines.)
Review security issues by navigating to semmleportal.azurewebsites.net/lookup. It may take up to one day to process results.

This is not done yet (unless it happens automatically) and I cannot find any mention of LGTM in InnerEye to crib from.

The CodeQL Portal says "Only Visual Studio Team System and Azure Dev Ops URLs are supported" and will not upload a snapshot from GitHub

To ease local development and testing, instead of requiring a package to be built, copy the contents of the "src" folder into the test folder.

Ensure that all files have copyright notices, and that editors are set up to automatically insert them (PyCharm does it correctly on InnerEye)

You must run the following source code analysis tools:
CredScan
CodeQL (Semmle)
Component Governance Detection
The easiest way to run these tools is to add thems in your build pipeline in a Microsoft-managed Azure DevOps account.

For CodeQL, please ensure the following (detailed instructions for CodeQL can be found here):
Select the source code language in the CodeQL task.
If your application was developed using multiple languages, add multiple CodeQL tasks.
Define the build variable LGTM.UploadSnapshot=true.
Configure the build to allow scripts to access OAuth token.
If the code is hosted in Github, create Azure DevOps PAT token with code read scope for dev.azure.com/Microsoft (or ‘all’) organization and set the local task variable System_AccessToken with it. (Note: This only works for YAML-based pipelines.)
Review security issues by navigating to semmleportal.azurewebsites.net/lookup. It may take up to one day to process results.

• Branch on InnerEye-DeepLearning: antonsc/himl. This code uses hi-ml as a submodule
• Remove the submodule hi-ml
• Add hi-ml as a package in environment.yml
• At present, there are two test failures, that should vanish when switching to hi-ml as a package:
  • test_register_and_score_model in the TrainEnsemble leg
  • test_submit_for_inference in the TrainInAzureMLViaSubmodule leg

AML Dataset's get_by_name sometimes returns ServiceException 204 - 'unknown error'

Examples of failed runs:

• https://github.com/microsoft/hi-ml/runs/3342537508?check_suite_focus=true
• https://github.com/microsoft/hi-ml/runs/3349433791?check_suite_focus=true

All arguments that point to files are presently accepting Path objects. However, many people still use strings for handling file names. Ensure that all arguments are happily accepting PathOrString.

From old repo, copy over everything that makes sense. This would include:

• flake8 and mypy checks as github workflows
• Running pytest and publishing pytest results to ADO
• Running credscan and component governance
• Copying issues to Azure DevOps

(Separated out from Issue #2)

Our code in setup.py will trigger with new tags. setuptools.setup will reject tags that are not release versions but we could do more to make that explicit by checking for the leading "v".

Also when we tag releases as, say, "v0.1.1" the leading "v" is carried through setuptools.setup so it becomes part of the pip test download

Successfully installed pip-21.2.4
Collecting hi-ml==v0.1.0
Downloading hi_ml-0.1.0-py3-none-any.whl (25 kB)

(from here)

This works, but it would be cleaner to submit the version number using the public version identifier format mandated in PEP 440, i.e. without the leading "v"

• Add vscode config files
• Add more testing to makefile
• Add code coverage comment bot

sphinx/readthedocs?

submit_if_needed needs arguments for

• default data store
• input datasets
• output datasets

Datasets can be specified either programmatically via a DatasetConfig, or as a string. DatasetConfig should contain

• dataset name. If does not yet exist, create
• datastore name - if missing, use the default data store
• version (optional) - only makes sense for input datasets
• should the dataset be mounted or downloaded when running in AML?
• folder location for mount or download when running in AML
• Optional: folder location to use when running outside AML

Datasets as strings:

• "foo": use dataset foo
• "foo:2": Use version 2 of dataset "foo"
• "mount:foo:/tmp/nix": Mount dataset at the given path. Create an equivalent for "download". Add options for versions

Depending on if the dataset is in the input or outputs list, we can create an input or output config from that.

We are not consistent about prefixing private functions with one underscore (module functions), or two (instance methods). That doesn't matter much for an internal project but as we hope to have external developers using these packages we should signal clearly the difference between private functions which should only be called by code inside the package, and public functions which may be useful to consumers of the package.

amlignore_path is not assigned if ignored_folders is empty:

    if ignored_folders:
        amlignore_path = snapshot_root_directory or Path.cwd()
        amlignore_path = amlignore_path / ".amlignore"
        lines_to_append = [str(path) for path in ignored_folders] if ignored_folders else []
    with append_to_amlignore(
            amlignore=amlignore_path,
            lines_to_append=lines_to_append):

Copy the essence of DICOM-RT package to here

from old code:

    # AzureML seems to sometimes expect the entry script path in Linux format, hence convert to posix path
    entry_script_relative_path = source_config.entry_script.relative_to(source_config.root_folder).as_posix()

In the same washup:

    # Use blob storage for storing the source, rather than the FileShares section of the storage account.
    run_config.source_directory_data_store = workspace.datastores.get(WORKSPACE_DEFAULT_BLOB_STORE_NAME).name

• Start with the simplest possible script. Explain what is being uploaded and executed
• Then add components one by one: Conda environment, pip extra index, docker
• Use of existing environment (can skip that)
• Input datasets
• Output datasets
• From the InnerEye documentation, copy over the section that explains how we are dealing with datasets. Extend and polish that. What are AML datasets, how do they relate to datasets in blob storage?

Introduce a while loop similar to publishing to test.pypi to check that the version downloaded is the same as the version just uploaded

• Automatically find config.json and environment.yml in current folder or any of the parent folders (stop going up when going out of a folder that is in PythonPath)

Add a new file: run_requirements.txt with the package run requirements.
Parse this and add it to the setup.py install_requires array.
Add a new shell script to pip install all the requirements for ease of development.

• Break submit_to_azure_if_needed into smaller functions
• Write unit tests for those smaller functions
• Write unit tests for the various utility functions they rely on (copied from Inner Eye where possible)
• Where appropriate (hopefully everywhere!) mark as @pytest.mark.fast

To publish a new package to PyPi (i.e. the public package repository, not the test one) we run these commands:

make clean
make build
twine upload dist/*

That runs setup.py. To get local testing working we added code to setup.py that checks whether it is running in GitHub and if it is not then it changes the post number to a string of nine random digits. Unfortunately that code runs when packaging for real, i.e. not as part of testing, and so instead of a build numbers like 0.0.1post1, 0.0.1post2 , 0.0.1post3 etc. we get ones like 0.0.1post 5725449762

We should get flake, mypy, and unit test results all at once, rather than stopping already after failing mypy - if I submit a change, I should get a clear indication about ALL the things that are wrong.

At present, we use commandline arguments, and always tag runs with git commit information. We can do that in hi-ml too, but maybe have a switch to turn that off (no run tagging)

When running the unit tests, some folders are created using the pytest tmp_path fixture and the src folder is copied into them. This means that the coverage tool things they are different to the installed package. When running as a test in github action then the package is already installed, so this should not be necessary. When running locally it should be possible to install the src folder as an editable package, with the -e option and the tests still run.

To get things started, upload and run a xscript that just prints out a message handed in as an argument using our new submit_to_azure_if_needed function

Javier pointed out that our tagline, Microsoft Health Intelligence AzureML helpers, on https://pypi.org/manage/project/hi-ml/releases/ is too generic.

In the Makefile, we now have code to do mypy, flake, environment building. Use those as building blocks in the PR build. This way we can ensure that the local dev environment is the same as used in the cloud.

Move this segment into the AzureML layer:

        # For PR builds where we wait for job completion, the job must have ended in a COMPLETED state.
        if self.azure_config.wait_for_completion and not is_run_and_child_runs_completed(azure_run):
            raise ValueError(f"Run {azure_run.id} in experiment {azure_run.experiment.name} or one of its child "
                             "runs failed.")

• package_setup_and_hacks - parts of it should be called by submit_if_needed (matplotlib, MKL)
• is_global_rank_zero()
• set_environment_variables_for_multi_node: This should probably be renamed to include something around Pytorch Lightning, because we set environment variables in a way that the PL trainer needs.

Helper functions to use in code to download a file from run (that work seamlessly in local runs and in AML) – used for downloading a checkpoint

• In AzureML, they should take authentication/workspace from the current run context
• Outside AzureML, it needs a config.json object

Unfortunately the build code coverage is still pointing at "src" when evaluating the package. Change it to point at "health"

• download via wildcard from the most recent run, or a given run
• downloaded folder structure should include the run, to have multiple results side by side easily
• Install as executable when installing the package, see #59

Add unit tests for dataset downloading/mounting for input and output.

The github action, build-test-pr.yml, and makefile, both call mypy directly. Change them both to call mypy_runner instead.

Also change the build dependencies to publish to test.pypi only dependent on pytest

(Separated out from Issue #2)

• Ensure that the Readme file on pypi is correct and meaningful
• Describe how to set up a dev environment (create conda env/pip)
• How to set up and use a Service Principal
• How does device login work (and when does it not work)
• Describe how to do a release (pep guidelines for versions)