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

Include standard data libraries in Python package #1237

Merged

Conversation

kwokcb
Copy link
Contributor

@kwokcb kwokcb commented Feb 10, 2023

Package the standard definition libraries as part of the Python package.
This removes the need for users to have to always search for the libraries, and the possibility to mistakenly use libraries which do not match the Python distribution.

  • An additional MANIFEST.in is included to add in the libraries folder if setuptools is used instead of distutils.core.
  • A convenience function getDefaultDataSearchPath() will provide a default search path to find libraries.
  • A convenience function getDefaultDataLibraryFolders() will return a list containing data library folder names.
  • The mxvalidate.py script has been update use the new API with loadLibraries(). The simplest usage would be
    this one-liner:
# Create the library document
stdlib = mx.createDocument()
# Load in the default data libraries
 mx.loadLibraries(mx.getDefaultDataLibraryFolders(), mx.getDefaultDataSearchPath(), stdlib)   

Add in a conventience function to return the library to the 'libraries' folder.
@kwokcb
Copy link
Contributor Author

kwokcb commented Feb 10, 2023

@jstone-lucasfilm , this is follow-up to your suggestion to allow std libs to be part of the Python package. Seems useful to
always have this available as it does not force usage of these libraries, but does provide consistency in releases. It is left it as a build option. Let me know what you think.

python/MaterialX/MANIFEST.in Show resolved Hide resolved
python/MaterialX/main.py Outdated Show resolved Hide resolved
python/Scripts/mxvalidate.py Outdated Show resolved Hide resolved
libraries/CMakeLists.txt Outdated Show resolved Hide resolved
@jstone-lucasfilm
Copy link
Member

Thanks for putting this proposal together, @kwokcb, and I think this is a great direction for the future. I wonder if we might ultimately want the data libraries to be included in all MaterialX Python builds, rather than making this optional? I'm interested in thoughts from other developers on this question, and we may want to discuss this at a future MaterialX TSC meeting.

@jstone-lucasfilm jstone-lucasfilm changed the title Add option to install "standard libraries" in Python package Add option to include data libraries in Python package Feb 10, 2023
@kwokcb
Copy link
Contributor Author

kwokcb commented Feb 13, 2023

Hi @jstone-lucasfilm ,
I had originally wrote this as always being there.

The pros I see are:

  1. What has been discussed before. The libraries can always be found.
  2. The Python package is self-contained. No additional "side-car" files are required.
  3. The libraries are always consistent with the release. I'm sure folks could accidently grab the wrong libraries.
  4. No more need for search libraries -- and possibly getting the wrong libraries due to searching.
  5. This could help with USD integration as it's a set of libraries that is more discoverable.

Cons are:
A. It's static -- but this is offset by the fact that they are not automatically loaded so a developer can still load
in their own std libraries.
B. There is a degree of overhead as they are duplicated -- but this should be offset by the fact that they have to exist somewhere. Also you don't need the copies for C++ usage if your just working in Python.

In all, I think the pros out-weigh the cons (especially 1-3) , so I vote we not have an option at first, and then if there are issues
raised by developers add in a new option.

@jstone-lucasfilm
Copy link
Member

We had a chance to discuss packaging of data libraries with MaterialX Python at today's MaterialX TSC meeting, and the idea had consistent support from the members of the TSC. Here are the full meeting notes for those who are interested:

https://academysoftwarefdn.slack.com/archives/C0230LWBE2X/p1676403077275179

@jstone-lucasfilm
Copy link
Member

@kwokcb Taking this proposal forward, my sense is that we can safely remove the MATERIALX_INCLUDE_PYTHON_STDLIB option, and include the data libraries in all MaterialX Python packages. This would allow MaterialX Python developers to rely upon it being present, starting with the version of MaterialX in which this proposal is ultimately released.

@kwokcb kwokcb changed the title Add option to include data libraries in Python package Include standard definition libraries in Python package Mar 24, 2023
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

This looks great to me, thanks @kwokcb, and I'll merge the feature so that it's included in the upcoming MaterialX 1.38.7.

@jstone-lucasfilm jstone-lucasfilm changed the title Include standard definition libraries in Python package Include standard data libraries in Python package Apr 5, 2023
@jstone-lucasfilm jstone-lucasfilm merged commit 1d7d2e0 into AcademySoftwareFoundation:main Apr 5, 2023
12 checks passed
@kwokcb kwokcb deleted the install_stdlib_python branch April 11, 2023 21:26
jstone-lucasfilm pushed a commit that referenced this pull request Aug 25, 2023
Since the addition of #1237, we now create a python folder, with a copy of the stdlibs, even if we're not building python.

This is at best redundant, and in my particular case, caused a problem in a downstream build script.
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
…ndation#1237)

Package the standard definition libraries as part of the Python package. This removes the need for users to have to always search for the libraries, and the possibility to mistakenly use libraries which do not match the Python distribution.
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
…undation#1437)

Since the addition of AcademySoftwareFoundation#1237, we now create a python folder, with a copy of the stdlibs, even if we're not building python.

This is at best redundant, and in my particular case, caused a problem in a downstream build script.
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

2 participants