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

feat: Adding RestrictedFeatures Support to the Python Frontend Bindings #7775

Open
wants to merge 57 commits into
base: main
Choose a base branch
from

Conversation

KrishnanPrash
Copy link
Contributor

@KrishnanPrash KrishnanPrash commented Nov 8, 2024

What does the PR do?

This PR provides Restricted Features support to the python frontend bindings, a.k.a tritonfrontend.
This PR introduces 3 new classes to the tritonfrontend package:

  1. Feature: 1-to-1 mapping of RestrictedCategory Enum
  2. FeatureGroup: Pydantic @dataclass that stores (key, value, features) information while performing type validation.
  3. RestrictedFeatures: Store collections of FeatureGroup instances, apply an additional layer of validation to check for collisions (ensuring that each feature belongs to only one group), and serialize the data into a JSON string.

With these changes, we can pass a RestrictedFeatures to the KServeHttp and KServeGrpc frontends, which looks along the lines of:

>>> import tritonserver
>>> from tritonfrontend import KServeHttp, KServeGrpc
>>> from tritonfrontend import RestrictedFeatures
>>> server = tritonserver.Server(...).start(wait_until_ready=True)

>>> rf = RestrictedFeatures(...)

>>> http_options = KServeHttp.Options(restricted_features=rf)
>>> http_service = KServeHttp(server, http_options)
>>> http_service.start()

>>> grpc_options = KServeGrpc.Options(restricted_features=rf)
>>> grpc_service = KServeGrpc(server, grpc_options)
>>> grpc_service.start()

A RestrictedFeatures instance can be created with:

>>> from tritonfrontend import Feature, FeatureGroup, RestrictedFeatures
>>> admin_group = FeatureGroup(key="admin-key", value="admin-value", features=[Feature.HEALTH, Feature.METADATA])
>>> infer_group = FeatureGroup("infer-key", "infer-value", [Feature.INFERENCE])

>>> rf = RestrictedFeatures([admin_group, infer_group])

>>> model_group = FeatureGroup("model-key", "model-value", [Feature.MODEL_REPOSITORY])
>>> rf.add_feature_group(model_group)

>>> rf.create_feature_group("stat-key", "stat-value", [Feature.STATISTICS])
>>> rf
[  
   {"key": "admin-key", "value": "admin-value", "features": ["health"]},
   {"key": "admin-key", "value": "admin-value", "features": ["metadata"]},  
   {"key": "infer-key", "value": "infer-value", "features": ["inference"]}, 
   {"key": "model-key", "value": "model-value", "features": ["model-repository"]}, 
   {"key": "stat-key", "value": "stat-value", "features": ["statistics"]}
]

Where should the reviewer start?

Take a look at api/_restricted_features.py and tritonfrontend.h

Test plan:

In L0_python_api/test_kserve.py added:

  • test_correct_rf_parameters() and test_wrong_rf_parameters(): Tests for valid/invalid arguments passed/selected for Feature, FeatureGroup and RestrictedFeature
    To test if restricted features are working correctly with the frontends:
  • test_restricted_features(): Restricts inference. Then, sends an inference request with a correct and incorrect header to verify proper functionality.

CI Pipeline ID: 20189649

Background

For more information about restricted features support in Triton, please take a look at this: [Link]

Additonal Changes Made in this PR

  • Cleaned up includes in tritonfrontend.h and tritonfrontend_pybind.cc.
  • Added support to @handle_triton_error for non-void functions.
  • Added support to pass headers in send_and_test_inference_identity() so that we can re-use utility function for restricted features testing.
  • Skipping tests that use KServeGrpc and tritonclient.grpc because of lack of fork() support provided by python cygrpc. More information in DLIS-7215.

@KrishnanPrash KrishnanPrash self-assigned this Nov 8, 2024
@KrishnanPrash KrishnanPrash requested a review from GuanLuo December 2, 2024 17:09
qa/L0_python_api/test_kserve.py Outdated Show resolved Hide resolved
qa/L0_python_api/test_kserve.py Outdated Show resolved Hide resolved
qa/L0_python_api/test_kserve.py Outdated Show resolved Hide resolved
qa/L0_python_api/test_kserve.py Show resolved Hide resolved
qa/L0_python_api/test_kserve.py Outdated Show resolved Hide resolved
qa/L0_python_api/test_kserve.py Outdated Show resolved Hide resolved
qa/L0_python_api/test_kserve.py Outdated Show resolved Hide resolved
qa/L0_python_api/test_kserve.py Show resolved Hide resolved
qa/L0_python_api/test_kserve.py Show resolved Hide resolved
src/python/tritonfrontend/_api/_restricted_features.py Outdated Show resolved Hide resolved
Comment on lines 54 to 57
# TODO: [DLIS-7215] Run tritonclient.grpc as separate process
# Currently, when we run tritonclient.grpc with tritonserver in the same process,
# When a fork() is called by tritonserver on model load, lack of support in cygrpc
# causes the python process to abort/crash without being able to be caught by pytest.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate or share references? I don't understand how the client is related to server/model fork, and what "lack of support in cygrpc" means

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment providing more context into how cygrpc relates to directly imported python packages. Added link for further reading and reference.

qa/L0_python_api/test.sh Outdated Show resolved Hide resolved
# it will non-deterministically abort/crash without being able to be caught by pytest.
# This is because fork() is called by tritonserver on model load,
# which attempts to fork the imported libraries and their internal states,
# and cygrpc (dependency of tritonclient.grpc) does not officially support fork().
Copy link
Contributor

@rmccorm4 rmccorm4 Dec 20, 2024

Choose a reason for hiding this comment

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

However, if the application only instantiate gRPC Python objects after calling fork(), then fork() will work normally, since there is no C extension binding at this point.

If the claim is that we're calling fork() during model load, aren't we instantiating grpc (client) after the fork() if server/service have already started up?

Can you ever reproduce this by only running a single test case repeatedly? If not, it's possible it's coming from the threshold between test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From, my understanding it is an internal state set upon import:

import tritonclient.grpc as grpcclient

while True:
     server = utils.startup_server()
     sleep(1)
     utils.teardown_server(server)

# This is because fork() is called by tritonserver on model load,
# which attempts to fork the imported libraries and their internal states,
# and cygrpc (dependency of tritonclient.grpc) does not officially support fork().
# Reference: https://github.com/grpc/grpc/blob/master/doc/fork_support.md
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't you just set env var as mentioned here so that the tests work with fork? https://github.com/grpc/grpc/blob/master/doc/fork_support.md#current-status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Attempted to set and run test suite locally, but the test cases were still failing.

rf.create_feature_group(
key=42, value="Secret to the Universe", features=[Feature.HEALTH]
)
with pytest.raises(Exception):
Copy link
Contributor

Choose a reason for hiding this comment

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

Check if exception enclose informative message

…btxt

Co-authored-by: GuanLuo <41310872+GuanLuo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants