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

add compile_method flag and add other framework artifact types #40

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

rbavery
Copy link
Collaborator

@rbavery rbavery commented Oct 30, 2024

Description

This adds a compile_method to the MLM Asset which identifies if the model artifact has related jit or aot compilation workflows applied to it (either when saving the artifact for aot or during runtime for jit).

It also reorganizes a lot of framework specific recommendations for describing model export formats to the best-practices.md . More descriptions for Tensorflow and Keras specific export formats are added as well.

Related Issue

Type of Change

  • 📚 Examples, docs, tutorials or dependencies update;
  • 🔧 Bug fix (non-breaking change which fixes an issue);
  • 🥂 Improvement (non-breaking change which improves an existing feature);
  • 🚀 New feature (non-breaking change which adds functionality);
  • 💥 Breaking change (fix or feature that would cause existing functionality to change);
  • 🔐 Security fix.

Checklist

  • I've read the CONTRIBUTING.md guide;
  • I've updated the code style using make check;
  • I've written tests for all new methods and classes that I created;
  • I've written the docstring in Google format for all the methods and classes that I used.

@rbavery rbavery changed the title add compile_method flag and add other framework artifact types [wip] add compile_method flag and add other framework artifact types Oct 30, 2024
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@rbavery
Copy link
Collaborator Author

rbavery commented Dec 13, 2024

@fmigneault I added mlm:compile_method to the JSON Schema but this seems to have broken validation of mlm:unknown in one of the tests.

make test

――――――――――――――――――――――――――――――――――――――――――――― test_mlm_no_undefined_prefixed_field_item_properties[item_raster_bands.json] ―――――――――――――――――――――――――――――――――――――――――――――
tests/test_schema.py:61: in test_mlm_no_undefined_prefixed_field_item_properties
    with pytest.raises(pystac.errors.STACValidationError) as exc:
E   Failed: DID NOT RAISE <class 'pystac.errors.STACValidationError'>

I went through and added mlm:compile_method wherever mlm:artifact_type exists since I think we want validation behavior to be mostly the same for these fields. The only section I didn't add was

    "HasArtifactType": {
      "$comment": "Used to check the artifact type property that is required by a Model Asset annotated by 'mlm:model' role.",
      "type": "object",
      "required": [
        "mlm:artifact_type"
      ],
      "properties": {
        "mlm:artifact_type": {
          "$ref": "#/$defs/mlm:artifact_type"
        }
      }
    },

Any tips on what I need to change in the schema? I want it to be allowed under the asset role, if it is present I want it validated to be a string, but unlike mlm:artifact_type I don't want it to be required if the mlm-model role is there.

@rbavery rbavery changed the title [wip] add compile_method flag and add other framework artifact types add compile_method flag and add other framework artifact types Dec 13, 2024
@rbavery rbavery requested a review from fmigneault December 13, 2024 19:08
CHANGELOG.md Outdated Show resolved Hide resolved
best-practices.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
json-schema/schema.json Outdated Show resolved Hide resolved
best-practices.md Outdated Show resolved Hide resolved
best-practices.md Outdated Show resolved Hide resolved
best-practices.md Outdated Show resolved Hide resolved
best-practices.md Outdated Show resolved Hide resolved
@rbavery rbavery requested a review from fmigneault December 13, 2024 23:48
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.

2 participants