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

AML-6 Model Manifest Compilation Integration #113

Merged
merged 9 commits into from Oct 15, 2021
Merged

Conversation

zeryx
Copy link
Contributor

@zeryx zeryx commented Oct 5, 2021

This adds an additional CLI function called compile, which triggers a model_manifest.json.lock file creation, which is used by the model manifest system and data immutability & governance process.

algo compile in an algorithm directory containing a model_manifest.json file will trigger a lock operation; which calculates the md5 checksum's of all model files, and the md5 checksum of the lockfile itself; reducing the likelihood of tampering. compile requires authentication with credentials that allows access to any data API files that are defined in the manifest.

As an example; please use the model_manifest.json defined in https://gist.github.com/zeryx/5ebd1a9072a66803abff6889e051b126; and make sure that your lockfile generated contains the same md5 hashes for all relevant model files.

the ADK sibling PR is located here: algorithmiaio/algorithmia-adk-python#7

Algorithmia/__main__.py Outdated Show resolved Hide resolved
Algorithmia/CLI.py Outdated Show resolved Hide resolved
Copy link
Contributor

@lemonez lemonez left a comment

Choose a reason for hiding this comment

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

see comments

@zeryx zeryx requested a review from lemonez October 7, 2021 15:09
lemonez
lemonez previously approved these changes Oct 7, 2021
Copy link
Contributor

@lemonez lemonez left a comment

Choose a reason for hiding this comment

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

nice

@@ -215,6 +219,9 @@ def main():
elif args.cmd == 'builds':
print(CLI().getBuildLogs(args.user, args.algo, client))

elif args.cmd == "lock":
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "freeze" is a fairly standard term for this action. Also the PR description doesn't match, it says algo compile

https://pip.pypa.io/en/stable/cli/pip_freeze/

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also prefer to use freeze for this concept. From a software eng perspective:

lock: infers a mutual-exclusion concept; as if it's a mechanism to restrict access to files among multiple processes, as in the lockfile concept in Linux to create semaphores
compile: infers bundling/transformation of something into a runnable/interpretable package to be used by other components.

If we go by the name freeze, then for the sake of consistency we can consider renaming the variables in the ADK such as manifest_lock_path, manifest_file.json.lock, etc. too. I'll put a note in the ADK PR for this.

@lemonez
Copy link
Contributor

lemonez commented Oct 7, 2021 via email

Algorithmia/__main__.py Outdated Show resolved Hide resolved
Algorithmia/util.py Outdated Show resolved Hide resolved
Algorithmia/__main__.py Outdated Show resolved Hide resolved
@@ -215,6 +219,9 @@ def main():
elif args.cmd == 'builds':
print(CLI().getBuildLogs(args.user, args.algo, client))

elif args.cmd == "lock":
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also prefer to use freeze for this concept. From a software eng perspective:

lock: infers a mutual-exclusion concept; as if it's a mechanism to restrict access to files among multiple processes, as in the lockfile concept in Linux to create semaphores
compile: infers bundling/transformation of something into a runnable/interpretable package to be used by other components.

If we go by the name freeze, then for the sake of consistency we can consider renaming the variables in the ADK such as manifest_lock_path, manifest_file.json.lock, etc. too. I'll put a note in the ADK PR for this.

@aslisabanci
Copy link
Contributor

Just noticed that the algo_inputs variable on this line contains an extra s at the end @zeryx, so that variable becomes unused - it should be algo_input. So this is a good moment to appreciate those compiled languages once more, since they don't allow these type of programming errors😎

@lemonez
Copy link
Contributor

lemonez commented Oct 11, 2021

Just noticed that the algo_inputs variable on this line contains an extra s at the end @zeryx, so that variable becomes unused - it should be algo_input. So this is a good moment to appreciate those compiled languages once more, since they don't allow these type of programming errors😎

Not to say we shouldn't appreciate compiled langs, but we could also probably turn the linting dial up a notch to catch some things too, and then we can keep appreciating Python and its versatility as well 🐍 😃

@zeryx zeryx merged commit 339d779 into develop Oct 15, 2021
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.

None yet

4 participants