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 11 commits into
base: main
Choose a base branch
from

Conversation

rbavery
Copy link
Collaborator

@rbavery rbavery commented Oct 30, 2024

Description

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
| `torch.jit.save` | A [`TorchScript`][pytorch-jit-script] model artifact obtained with one or more of the graph export options Torchscript Tracing and Torchscript Scripting. |
| `torch.export.save` | A model artifact storing an ExportedProgram obtained by [`torch.export.export`][pytorch-export] (i.e.: `.pt2`). |
| `TFSavedModel` | A [SavedModel][tf-save] from Tensorflow or Keras. |
| `Keras_v3` | Keras v3 is the [recommended format][keras-recommended] by the Tensorflow team. See this example to [save and load models][keras-example] and the update to date docs [disambiguating different save methods][keras-methods] in TF and Keras.. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as previous. Any reference function to be employed?
I would assume Keras on its own has a way to generate that file, regardless of how TF can reload it. Maybe: keras.saving.serialize_keras_object (https://keras.io/api/models/model_saving_apis/serialization_utils/) ?

extra . to remove at the end

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I renamed all these to point to specific functions and included an argument in one of them, tf.keras.Model.export(format='tf_saved_model') that is tensorflow specific. The other two functions always output the same artifact. Would prefer to use the high level function tf.keras.Model.export(format='tf_saved_model') as the artifact type since it is what users will typically interact with rather than a lower level function like keras.saving.serialize_keras_object

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