Resolved Python module import dependencies

[StefanHabel]

This patch adds pybind11::module::import() calls to MaterialX Python C extension modules
that depend on other MaterialX Python C extension modules.

The intention is to allow any of the MaterialX Python C extension modules to be imported
in any order.

A couple of the Python scripts are updated to remove unused MaterialX imports,
and to sort imports alphabetically (which is now possible as the modules import
their own dependencies).

My dev environment, for reference:

• Apple clang version 15.0.0 (clang-1500.0.40.1)
• Python 3.12.0 (v3.12.0:0fb18b02c8, Oct 2 2023, 09:45:56) [Clang 13.0.0 (clang-1300.0.29.30)] on darwin
• Sphinx 7.2.6
• cmake version 3.27.6

Split from #1567.

Update #342.

[jstone-lucasfilm]

This looks like a great improvement, thanks @StefanHabel!

I'm imagining that this change will allow us to simplify several of the import sequences in our example scripts. If this turns out to be the case, then we might consider relying upon these simplified import sequences to validate these improvements going forward, rather than having a dedicated imports.py script in our unit tests.

[StefanHabel]

This looks like a great improvement, thanks @StefanHabel!

I'm imagining that this change will allow us to simplify several of the import sequences in our example scripts. If this turns out to be the case, then we might consider relying upon these simplified import sequences to validate these improvements going forward, rather than having a dedicated imports.py script in our unit tests.

Cheers @jstone-lucasfilm!

I see, there appear to be a couple of unused imports in some of those scripts, which were likely necessary previously due to these import dependencies, e.g.

from MaterialX import PyMaterialXGenShader
from MaterialX import PyMaterialXGenGlsl

in https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/python/Scripts/baketextures.py#L9-L10

We are likely able to remove those unused imports from those script files, given that the imported and used modules should import their own dependencies.

I can give this a go locally.

If this works out (which I expect it will), would we want to remove the new imports.py unittest script from this PR again?

[StefanHabel]

(Meta: I'm just realizing that GitHub does not support replying to a comment, other than by quoting from a comment 😑)

I looked into this for a little bit. Only baketextures.py and translateshader.py contain unused imports of MaterialX Python modules, which is flagged by flake8:

python/Scripts/baketextures.py:9:1: F401 'MaterialX.PyMaterialXGenShader' imported but unused
python/Scripts/baketextures.py:10:1: F401 'MaterialX.PyMaterialXGenGlsl' imported but unused
python/Scripts/translateshader.py:12:1: F401 'MaterialX.PyMaterialXGenGlsl as ms_gen_glsl' imported but unused
python/Scripts/translateshader.py:16:5: F401 'MaterialX.PyMaterialXGenMsl as ms_gen_msl' imported but unused

The MaterialX imports in generateshader.py, for example, are all used:

import MaterialX as mx
import MaterialX.PyMaterialXGenShader as mx_gen_shader
import MaterialX.PyMaterialXGenGlsl as mx_gen_glsl
import MaterialX.PyMaterialXGenOsl as mx_gen_osl
import MaterialX.PyMaterialXGenMdl as mx_gen_mdl
import MaterialX.PyMaterialXGenMsl as mx_gen_msl

We could now sort them alphabetically, if we wanted to, but as they're all used, these separate imports are all still required:

import MaterialX as mx
import MaterialX.PyMaterialXGenGlsl as mx_gen_glsl
import MaterialX.PyMaterialXGenMdl as mx_gen_mdl
import MaterialX.PyMaterialXGenMsl as mx_gen_msl
import MaterialX.PyMaterialXGenOsl as mx_gen_osl
import MaterialX.PyMaterialXGenShader as mx_gen_shader

Would the idea be to simplify these imports?

[jstone-lucasfilm]

Those are good points, @StefanHabel, and it's true that a dedicated imports.py script provides more complete coverage of our import dependencies.

Maybe the best answer is to keep this new script in your pull request, but to additionally simplify the existing baketextures.py and translateshader.py scripts, so that we're validating the new import logic in common real-world situations.

[StefanHabel]

I've added a commit to remove the unused MaterialX imports: 1cd726c

It looks like two checks on Ubuntu have failed with an "Internal Server Error".

For example, here's what the error looks like in the raw log of the Python Wheels run for Python 3.8 on Ubuntu:

2023-11-08T21:20:33.4903513Z ##[group]Starting container image quay.io/pypa/manylinux2014_x86_64:2023-06-08-775c518...
2023-11-08T21:20:33.4908748Z 
2023-11-08T21:20:33.4909864Z info: This container will host the build for cp38-manylinux_x86_64...
2023-11-08T21:20:33.5240738Z Unable to find image 'quay.io/pypa/manylinux2014_x86_64:2023-06-08-775c518' locally
2023-11-08T21:20:35.3918662Z Error response from daemon: received unexpected HTTP status: 500 Internal Server Error
2023-11-08T21:20:35.3935406Z ##[endgroup]
2023-11-08T21:20:35.3937142Z                                                               �[31m✕ �[0m1.90s
2023-11-08T21:20:35.3980029Z ##[error]Command ['docker', 'create', '--env=CIBUILDWHEEL', '--name=cibuildwheel-82480035-fa3f-4a2c-aacd-4914de79d319', '--interactive', '--volume=/:/host', 'quay.io/pypa/manylinux2014_x86_64:2023-06-08-775c518', '/bin/bash'] failed with code 1. None
2023-11-08T21:20:35.4036103Z 
2023-11-08T21:20:35.4503620Z ##[error]Process completed with exit code 1.

For comparison, here's what the relevant bit from the log of the successful run of Python 3.7 on Ubuntu looked like:

2023-11-08T21:20:22.6544629Z ##[group]Starting container image quay.io/pypa/manylinux2014_x86_64:2023-06-08-775c518...
2023-11-08T21:20:22.6545357Z 
2023-11-08T21:20:22.6546086Z info: This container will host the build for cp37-manylinux_x86_64...
2023-11-08T21:20:22.6692208Z Unable to find image 'quay.io/pypa/manylinux2014_x86_64:2023-06-08-775c518' locally
2023-11-08T21:20:24.6258746Z 2023-06-08-775c518: Pulling from pypa/manylinux2014_x86_64
2023-11-08T21:20:24.6268089Z 2d473b07cdd5: Pulling fs layer
2023-11-08T21:20:24.6269198Z e37af20db26c: Pulling fs layer
2023-11-08T21:20:24.6270322Z f26ecc05ca7e: Pulling fs layer

@jstone-lucasfilm Assuming the two failed checks are random server failures, is there a way to re-run the checks, perhaps only the two that failed?

[jstone-lucasfilm]

@StefanHabel Done!

[jstone-lucasfilm]

@StefanHabel This looks great to me, and my sense is that your simplifications and reordering of imports in our example scripts are now sufficient to validate the improvements that you've made, so I'd say that it's fine to omit imports.py from the repository after all. I can go ahead and propose this change, so that we're just looking at the minimal set of refinements for this pull request.

[StefanHabel]

Thanks @jstone-lucasfilm !

I've edited the description of the PR to remove the notes about the unittest.