Add python bindings for `UsdValidator`, `UsdValidatorSuite` and `UsdValidationRegistry` #3242

roggiezhang-nv · 2024-08-20T02:46:21Z

Description of Change(s)

Add python bindings for UsdValidator, UsdValidatorSuite and UsdValidationRegistry.

It is stacked on #3223, #3232, and #3236 for adding Python bindings for validator framework.

Fixes Issue(s)

I have verified that all unit tests pass with the proposed changes

I have submitted a signed Contributor License Agreement

roggiezhang-nv · 2024-08-20T02:47:59Z

@nvmkuruc for vis.

jesschimein · 2024-08-20T22:03:56Z

Filed as internal issue #USD-10006

jesschimein · 2024-08-20T22:03:57Z

/AzurePipelines run

azure-pipelines · 2024-08-20T22:04:08Z

Azure Pipelines successfully started running 1 pipeline(s).

jesschimein · 2024-08-21T17:17:14Z

/AzurePipelines run

azure-pipelines · 2024-08-21T17:17:24Z

Azure Pipelines successfully started running 1 pipeline(s).

tallytalwar · 2024-08-21T22:44:19Z

I haven't started syncing this in yet, so will it be possible for you to follow the naming convention and 80 column line width, etc for commits specific for this PR (not the ones on which this is based)?

tallytalwar · 2024-08-21T23:15:09Z

pxr/usd/usd/testenv/testUsdValidationRegistryPy.py

+        Usd._register_test_validators()
+
+    def test_query_validators_and_suites(self):
+        validation_registry = Usd.ValidationRegistry()


The variable names also need to do follow camelCase convention. Thanks

I'll fix them.

Thanks @roggiezhang-nv.

tallytalwar · 2024-08-22T04:04:45Z

pxr/usd/usd/testenv/testUsdValidationRegistryPy.py

Thanks @roggiezhang-nv

I am not sure if it satisfies the format. I'd hope there is a format guideline or style file so we can enforce the same format.

Unfortunately we do not have a format enforcing (clang format equivalent) thing right now. But most of the coding conventions are documented here: https://openusd.org/release/api/_page__coding__guidelines.html

A change soon will make it more front and center when filing a PR.

jesschimein · 2024-08-22T16:47:23Z

/AzurePipelines run

azure-pipelines · 2024-08-22T16:47:32Z

Azure Pipelines successfully started running 1 pipeline(s).

tallytalwar

Took a pass at this. And I am not able to understand why we will want to restrict python bindings for registering validators. In my initial proposal I also provided an example I think of a usecase for this.

I think since validation registration just require callbacks, python cb can be mapped to the appropriate cpp APIs for the wrapping?

Kindly let me know if there are reasons I am failing to understand here. Thanks

tallytalwar · 2024-08-22T21:50:43Z

pxr/usd/usd/wrapValidationRegistry.cpp

+}
+
+// Test only for registering validators as no APIs are exposed to register
+// validators from python.


I don't think this should be the case. We should definitely expose validator registration from python. I will like to hear thoughts on why this restrictions is being placed? Thanks

pxr/usd/usd/wrapValidationRegistry.cpp

tallytalwar · 2024-08-22T21:56:09Z

pxr/usd/usd/wrapValidationRegistry.cpp

+             return_value_policy<TfPySequenceToList>(), (args("schemaTypes")));
+
+    // For python testing only
+    def("_register_test_validators", &_RegisterTestValidators);


I think we shouldn't be doing this here. Once we have APIs wrapped for registering of various validators, we should have the test code register the validators for its purpose, instead of making this wrapper code handle any test specific wrapping.

pxr/usd/usd/wrapValidationRegistry.cpp

nvmkuruc · 2024-08-22T23:08:51Z

Took a pass at this. And I am not able to understand why we will want to restrict python bindings for registering validators. In my initial proposal I also provided an example I think of a usecase for this.

I think since validation registration just require callbacks, python cb can be mapped to the appropriate cpp APIs for the wrapping?

Kindly let me know if there are reasons I am failing to understand here. Thanks

We thought the GIL warrants additional consideration when scheduling and running validators. We're not necessarily opposed to registering validators in python, but didn't want to do it on the initial pass.

tallytalwar · 2024-08-22T23:23:45Z

We thought the GIL warrants additional consideration when scheduling and running validators. We're not necessarily opposed registering validators in python, but didn't want to do it on the initial pass.

I see in that case, maybe lets remove the test code from the wrapper. I am just starting work on ValidationContext / Running of validators, so can also take a stab at this when this work is all done.

nvmkuruc · 2024-08-22T23:27:43Z

We thought the GIL warrants additional consideration when scheduling and running validators. We're not necessarily opposed registering validators in python, but didn't want to do it on the initial pass.

I see in that case, maybe lets remove the test code from the wrapper. I am just starting work on ValidationContext / Running of validators, so can also take a stab at this when this work is all done.

We could consider keeping _registerTest for now just for coverage and then revise once the ValidationContext method is complete.

roggiezhang-nv · 2024-08-23T01:51:45Z

We thought the GIL warrants additional consideration when scheduling and running validators. We're not necessarily opposed registering validators in python, but didn't want to do it on the initial pass.

I see in that case, maybe lets remove the test code from the wrapper. I am just starting work on ValidationContext / Running of validators, so can also take a stab at this when this work is all done.

I think Matt replied all those questions. Let me know if I have anything else to fix.

tallytalwar · 2024-08-23T07:35:52Z

We thought the GIL warrants additional consideration when scheduling and running validators. We're not necessarily opposed to registering validators in python, but didn't want to do it on the initial pass.

So having a chat with Alex and he mentioned about the use of TfPyFunctionFromPython, which takes care of acquiring/locking and releasing of GIL when appropriate python method is called and I think something along these lines should work:

void
_RegisterPluginPrimValidator(
        const TfToken &validatorName, UsdValidatePrimTaskFn primTaskFn)
{
    UsdValidationRegistry::GetInstance().RegisterPluginValidator(
        validatorName, primTaskFn);
}

void
_RegisterPrimValidator(
        boost::python::object metadata, UsdValidatePrimTaskFn primTaskFn)
{
    const UsdValidatorMetadata validatorMetadata = 
            boost::python::extract<UsdValidatorMetadata>(metadata);
    UsdValidationRegistry::GetInstance().RegisterValidator(
        validatorMetadata, primTaskFn);
}

void wrapUsdValidationRegistry()
{
  // Used with RegisterPluginPrimValidator, RegisterPluginStageValidator,
  // RegisterPluginPrimValidator, RegisterLayerValidator, RegisterStageValidator
  // and RegisterPrimValidator
  TfPyFunctionFromPython<UsdValidationErrorVector(const UsdPrim &)>();

  class_<UsdValidationRegistry, boost::noncopyable>("ValidationRegistry",
                                                    no_init)
      .def("__new__", &_GetRegistrySingleton,
           return_value_policy<reference_existing_object>())
      .staticmethod("__new__")
      .def("__init__", raw_function(_DummyInit))
      .def("RegisterPluginPrimValidator", &_RegisterPluginPrimValidator,
           (arg("validatorName"), arg("primTaskFn") = object()))
      .def("RegisterPrimValidator", &_RegisterPrimValidator,
           (arg("metadata") = object(), arg("primTaskFn") = object()))
      .def("HasValidator", &UsdValidationRegistry::HasValidator,
           (args("validatorName")))
....
}

Let me know if this makes sense?

roggiezhang-nv · 2024-08-23T08:18:25Z

We thought the GIL warrants additional consideration when scheduling and running validators. We're not necessarily opposed to registering validators in python, but didn't want to do it on the initial pass.

So having a chat with Alex and he mentioned about the use of TfPyFunctionFromPython, which takes care of acquiring/locking and releasing of GIL when appropriate python method is called and I think something along these lines should work:

void
_RegisterPluginPrimValidator(
        const TfToken &validatorName, UsdValidatePrimTaskFn primTaskFn)
{
    UsdValidationRegistry::GetInstance().RegisterPluginValidator(
        validatorName, primTaskFn);
}

void
_RegisterPrimValidator(
        boost::python::object metadata, UsdValidatePrimTaskFn primTaskFn)
{
    const UsdValidatorMetadata validatorMetadata = 
            boost::python::extract<UsdValidatorMetadata>(metadata);
    UsdValidationRegistry::GetInstance().RegisterValidator(
        validatorMetadata, primTaskFn);
}

void wrapUsdValidationRegistry()
{
  // Used with RegisterPluginPrimValidator, RegisterPluginStageValidator,
  // RegisterPluginPrimValidator, RegisterLayerValidator, RegisterStageValidator
  // and RegisterPrimValidator
  TfPyFunctionFromPython<UsdValidationErrorVector(const UsdPrim &)>();

  class_<UsdValidationRegistry, boost::noncopyable>("ValidationRegistry",
                                                    no_init)
      .def("__new__", &_GetRegistrySingleton,
           return_value_policy<reference_existing_object>())
      .staticmethod("__new__")
      .def("__init__", raw_function(_DummyInit))
      .def("RegisterPluginPrimValidator", &_RegisterPluginPrimValidator,
           (arg("validatorName"), arg("primTaskFn") = object()))
      .def("RegisterPrimValidator", &_RegisterPrimValidator,
           (arg("metadata") = object(), arg("primTaskFn") = object()))
      .def("HasValidator", &UsdValidationRegistry::HasValidator,
           (args("validatorName")))
....
}

Let me know if this makes sense?

It makes sense for sure. I think Matt concerns are more about how those validators are scheduled and running in a parallel execution environment. I'll let Matt to comment more.

roggiezhang-nv · 2024-08-26T01:39:39Z

@tallytalwar I rebased this branch to shrink the change history.

jesschimein · 2024-08-26T16:31:37Z

/AzurePipelines run

azure-pipelines · 2024-08-26T16:31:47Z

Azure Pipelines successfully started running 1 pipeline(s).

roggiezhang-nv · 2024-09-02T08:00:34Z

@tallytalwar @nvmkuruc I moved test cpp code into test plugin.

nvmkuruc · 2024-09-03T11:21:15Z

pxr/usd/usd/testenv/testUsdValidationRegistryPy.cpp

+
+TF_REGISTRY_FUNCTION(UsdValidationRegistry)
+{
+    fprintf(stderr, "test!!!!!\n");


Should this still be there?

Oh. Removed.

jesschimein · 2024-09-03T15:55:54Z

/AzurePipelines run

azure-pipelines · 2024-09-03T15:56:03Z

Azure Pipelines successfully started running 1 pipeline(s).

nvmkuruc · 2024-09-04T01:17:20Z

pxr/usd/usd/testenv/testUsdValidationRegistryPy.py

+    def setUpClass(cls) -> None:
+        Plug.Registry().RegisterPlugins(
+            os.path.dirname(__file__)
+            + "/UsdPlugins/lib/TestUsdValidationRegistryPy/Resources/"


Consider using pathlib or os.path.join to be safe.

nvmkuruc · 2024-09-04T01:26:39Z

pxr/usd/usd/wrapValidator.cpp

+        const UsdStagePtr &stage) const = &UsdValidator::Validate;
+    UsdValidationErrorVector (UsdValidator::*_ValidatePrim)(const UsdPrim &prim)
+        const = &UsdValidator::Validate;
+    class_<UsdValidator, boost::noncopyable>("Validator", no_init)


It might be helpful to have a __repr__ implementation.

You could consider returning UsdValidatorRegistry.GetOrLoadValidatorByName(<name>) if you're sure that the validator is in the registry. @tallytalwar for additional thoughts.

I think that makes sense to me, but to confirm, @nvmkuruc your suggestion will help a developer do:

validator = UsdValidatorRegistry.GetOrLoadValidatorByName("plug:validatorName") print(validator) UsdValidator(name='plug:validatorName')

Instead of printing the python object?

__repr__ added. Do we need to add method to stringify validator?

@tallytalwar Yup! It's analogous to Sdf.Layer.FindOrOpen.

I don't think stringify has much of a use case.

jesschimein · 2024-09-04T17:43:50Z

/AzurePipelines run

azure-pipelines · 2024-09-04T17:44:00Z

Azure Pipelines successfully started running 1 pipeline(s).

jesschimein · 2024-09-05T23:52:17Z

/AzurePipelines run

azure-pipelines · 2024-09-05T23:52:26Z

Azure Pipelines successfully started running 1 pipeline(s).

nvmkuruc · 2024-09-09T14:25:49Z

@tallytalwar @nvmkuruc I moved test cpp code into test plugin.

Thanks!

tallytalwar · 2024-09-09T21:58:12Z

Thanks @roggiezhang-nv for addressing notes and cleaning the PR. We just pushed an update to dev branch, which might require rebase/updates to this PR. Can you please update it? Thanks

roggiezhang-nv · 2024-09-10T00:30:21Z

Thanks @roggiezhang-nv for addressing notes and cleaning the PR. We just pushed an update to dev branch, which might require rebase/updates to this PR. Can you please update it? Thanks

Rebased.

jesschimein · 2024-09-10T16:03:47Z

/AzurePipelines run

azure-pipelines · 2024-09-10T16:03:57Z

Azure Pipelines successfully started running 1 pipeline(s).

comand · 2024-09-10T17:35:03Z

/AzurePipelines run

azure-pipelines · 2024-09-10T17:35:14Z

Azure Pipelines successfully started running 1 pipeline(s).

roggiezhang-nv changed the base branch from release to dev August 20, 2024 02:46

roggiezhang-nv force-pushed the add_bindings_for_validator_registry branch from df3bf80 to 3a639cb Compare August 20, 2024 02:51

roggiezhang-nv changed the title ~~Add python bindings for UsdValidationRegistry~~ Add python bindings for UsdValidator, UsdValidatorSuite and UsdValidationRegistry Aug 20, 2024

roggiezhang-nv force-pushed the add_bindings_for_validator_registry branch from 3a639cb to 96c4413 Compare August 21, 2024 00:37

tallytalwar reviewed Aug 21, 2024

View reviewed changes

tallytalwar reviewed Aug 22, 2024

View reviewed changes

roggiezhang-nv force-pushed the add_bindings_for_validator_registry branch from 5986aad to bbee580 Compare August 26, 2024 01:34

nvmkuruc reviewed Sep 3, 2024

View reviewed changes

roggiezhang-nv force-pushed the add_bindings_for_validator_registry branch from 80354ed to 8f0e7fc Compare September 3, 2024 11:33

nvmkuruc reviewed Sep 4, 2024

View reviewed changes

roggiezhang-nv force-pushed the add_bindings_for_validator_registry branch 2 times, most recently from 105f6db to 18bc714 Compare September 5, 2024 01:40

roggiezhang-nv added 2 commits September 10, 2024 08:29

Add python binding for UsdValidationRegistry

d423aff

Add __repr__ method for UsdValidator binding

a83d3ab

roggiezhang-nv force-pushed the add_bindings_for_validator_registry branch from 18bc714 to a83d3ab Compare September 10, 2024 00:29

pixar-oss merged commit e6600b8 into PixarAnimationStudios:dev Sep 19, 2024
3 of 5 checks passed

Add python bindings for UsdValidator, UsdValidatorSuite and UsdValidationRegistry #3242

Add python bindings for UsdValidator, UsdValidatorSuite and UsdValidationRegistry #3242

Conversation

roggiezhang-nv commented Aug 20, 2024 • edited Loading

Description of Change(s)

Fixes Issue(s)

roggiezhang-nv commented Aug 20, 2024

jesschimein commented Aug 20, 2024

jesschimein commented Aug 20, 2024

azure-pipelines bot commented Aug 20, 2024

jesschimein commented Aug 21, 2024

azure-pipelines bot commented Aug 21, 2024

tallytalwar commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesschimein commented Aug 22, 2024

azure-pipelines bot commented Aug 22, 2024

tallytalwar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvmkuruc commented Aug 22, 2024 • edited Loading

tallytalwar commented Aug 22, 2024

nvmkuruc commented Aug 22, 2024

roggiezhang-nv commented Aug 23, 2024

tallytalwar commented Aug 23, 2024

roggiezhang-nv commented Aug 23, 2024

roggiezhang-nv commented Aug 26, 2024

jesschimein commented Aug 26, 2024

azure-pipelines bot commented Aug 26, 2024

roggiezhang-nv commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesschimein commented Sep 3, 2024

azure-pipelines bot commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesschimein commented Sep 4, 2024

azure-pipelines bot commented Sep 4, 2024

jesschimein commented Sep 5, 2024

azure-pipelines bot commented Sep 5, 2024

nvmkuruc commented Sep 9, 2024

tallytalwar commented Sep 9, 2024

roggiezhang-nv commented Sep 10, 2024

jesschimein commented Sep 10, 2024

azure-pipelines bot commented Sep 10, 2024

comand commented Sep 10, 2024

azure-pipelines bot commented Sep 10, 2024

Add python bindings for `UsdValidator`, `UsdValidatorSuite` and `UsdValidationRegistry` #3242

Add python bindings for `UsdValidator`, `UsdValidatorSuite` and `UsdValidationRegistry` #3242

roggiezhang-nv commented Aug 20, 2024 •

edited

Loading

nvmkuruc commented Aug 22, 2024 •

edited

Loading