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 CI minimal build with all options disabled. Fix python binding code if sparse tensors are disabled. #9898

Merged
merged 6 commits into from
Dec 2, 2021

Conversation

skottmckay
Copy link
Contributor

Description:
Add CI builds that check the code compiles with all the optional features disabled.

Those are:

              --disable_exceptions \
              --disable_ml_ops \
              --cmake_extra_defines onnxruntime_DISABLE_SPARSE_TENSORS=ON \
                                    onnxruntime_DISABLE_OPTIONAL_TYPE=ON \
                                    onnxruntime_ENABLE_ORT_FORMAT_RUNTIME_GRAPH_OPTIMIZATION=OFF \
                                    onnxruntime_BUILD_UNIT_TESTS=OFF 

Fix the python binding code when DISABLE_SPARSE_TENSORS is defined.

Motivation and Context
Validate options used by 1P partners compile cleanly.

@@ -245,6 +252,70 @@ jobs:
--cmake_extra_defines onnxruntime_ENABLE_ORT_FORMAT_RUNTIME_GRAPH_OPTIMIZATION=ON
workingDirectory: $(Build.SourcesDirectory)

- script: git checkout -- .
Copy link
Contributor

Choose a reason for hiding this comment

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

this could probably move between 7a and 7b (possibly also after 7b in case we ever add more steps). steps 6a and 6b shouldn't update the code with any op exclusions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing it this way is a little more consistent as it matches the pattern of doing a cleanup, even if not strictly necessary, between each main step.

This way you only have to think about additional cleanup needs within a step (i.e. between the 'a' and 'b') and don't require knowledge across steps. I shouldn't need to know everything that happened within 6a and 6b to decide if cleanup is necessary or not before step 7.

Not needed between 7a and 7b as they're both disabling all ops.

Adding it after feels unnecessary as we may never add another step.

Copy link
Contributor

Choose a reason for hiding this comment

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

my interpretation of the pattern was that each numbered step (including 'a', 'b') that modifies the source cleans up after itself before the next numbered step.
I think this works though

Copy link
Member

@yuslepukhin yuslepukhin left a comment

Choose a reason for hiding this comment

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

:shipit:

@skottmckay skottmckay merged commit 912e50f into master Dec 2, 2021
@skottmckay skottmckay deleted the skottmckay/AddMinimalBuildWithAllOptionsDisabled branch December 2, 2021 20:56
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.

3 participants