Add bfloat to client utils #118

jbkyang-nvi · 2022-06-17T20:51:13Z

Add bfloat16 library to client utils.
Server PR to put bfloat16 pip library into SDK container: triton-inference-server/server#4521

rmccorm4 · 2022-06-21T21:17:55Z

src/python/library/tritonclient/utils/__init__.py

@@ -125,6 +125,7 @@ def debug_details(self):


 def np_to_triton_dtype(np_dtype):
+    from bfloat16 import bfloat16


Not asking for change here, but just a note for future reference - if we ever encounter any issues with importing this package - we can defer the import to only be when it is actually needed by placing this in the else block.

That way, no one will ever hit the bf16 import unless they're using a type not already covered by builtin numpy types that we support.

I think this is OK how it is for now though.

i.e.:

if np_dtype == bool: ... # If not covered by builtin numpy types, defer custom imports here to only be when needed else: from bfloat16 import bfloat16 if np_dtype == bfloat16: return "BF16" return None

If there is any issue with adding bfloat16 to our existing requirements.txt this could also be where we would output a warning/error to user that they have to install the package themself.

# If not covered by builtin numpy types, defer custom imports here to only be when needed else: try: from bfloat16 import bfloat16 except: print("ERROR: bfloat16 package is required, please install it: <pip install bfloat16>") return None if np_dtype == bfloat16: return "BF16" return None

Tabrizian

Do we need to add the dependency to requirements.txt file? If so, does this mean that the user will not be able to install tensorflow even if they don't use bfloat16?

rmccorm4 · 2022-06-21T21:23:32Z

src/python/library/tritonclient/utils/__init__.py

@@ -125,6 +125,7 @@ def debug_details(self):


 def np_to_triton_dtype(np_dtype):
+    from bfloat16 import bfloat16


Undoing my approval - agree with Iman, we probably need to add this to the requirements.txt - currently anyone calling this function will require the bf16 package.

jbkyang-nvi · 2022-06-22T00:42:09Z

@rmccorm4 @Tabrizian I made a workaround so that the user would not need to import bfloat16 and allow them to import tensorflow if they don't do that. Let me know what you think

rmccorm4 · 2022-06-22T00:56:14Z

src/python/library/tritonclient/utils/__init__.py

@@ -151,6 +151,9 @@ def np_to_triton_dtype(np_dtype):
        return "FP64"
    elif np_dtype == np.object_ or np_dtype.type == np.bytes_:
        return "BYTES"
+    elif (str(np_dtype) == "<class 'bfloat16'>"):
+        if np_dtype == bfloat16:


need the import here too right?

Also are we leaving it as an optional dependency user has to install then? If so we should document that somewhere noticeable.

If the dtype is already there, it means the user has already imported bfloat16 right? The user does not have to install bfloat16, it is already in the sdk container. I can add a comment here if they are doing their own thing in their container.

They will need to import bfloat16, which I assume they will do because they are converting a dtype to triton type and therefore already using it somewhere else

The user does not have to install bfloat16, it is already in the sdk container.

if they're using the client outside of the SDK container, they will. such as on jetson where we install wheel directly, right?

If the dtype is already there, it means the user has already imported bfloat16 right?

Your logic sounds reasonable that if they passed the type, it must already be imported - but I suspect a couple potential issues:

If they are using a different bf16 library that does the same thing (patching a numpy type), there may be a potential issue?

Also if for some reason they did import bfloat16 as bf16 in their code or used a differently named package, I believe this if np_dtype == bfloat16 would fail to resolve the name/package.

Added comments to address your concern with regards to user needing to install bfloat16

With regards to your concern about importing after the type->string check:
a. If the user is using another bfloat16 library, I think importing our own bfloat16 will not save us, since there definitely will be symbol conflicts. I think a better resolution would be to recommend the package to install. I don't like putting it in requirements.txt because it prevents the user from using tensorflow in the same script...
b. good point will do import inline as well...

rmccorm4

see comments

rmccorm4

Seems OK to me to save the import if and only if user is using bf16 type.

Not sure what the right move is on the requirements.txt to not include bf16 for users installing pip package / wheel directly.

I don't like putting it in requirements.txt because it prevents the user from using tensorflow in the same script

Wouldn't users trying to use Tensorflow in our SDK container run into the same problem if we're pre-installing it there for them?

It seems like we're being a little hacky to make this work with this particular package. Current options off the top of my head:

I don't think writing our own bf16 type is worth the effort/maintenance
Don't install this bfloat16 package by default in container/requirements.txt and make it clear to users somehow that they'll have to install it themselves if needed?
Forking/patching the bfloat16 package to simply fix the TF segfault issue ourselves? Maybe it's worth spending some cycles to find the root cause and re-evaluate?
Adding tensorflow pip package to the container/requirements to use tf.bfloat16 numpy type instead may make this less hacky, but looks like it would introduce ~500+MB which is quite large, so not ideal.
Not supporting BF16 natively in python client library for now (product decision)

What does everyone think? @jbkyang-nvi @Tabrizian @tanmayv25 @GuanLuo

jbkyang-nvi · 2022-06-23T21:58:55Z

Thought about it more and I think requirements.txt is reasonable. Also modified the import to include try/catch logic

re:

Forking/patching the bfloat16 package to simply fix the TF segfault issue ourselves? Maybe it's worth spending some cycles to find the root cause and re-evaluate?

I think this is a reasonable suggestion. I do think that we can do it as an additional ticket.

re:

Adding tensorflow pip package to the container/requirements to use tf.bfloat16 numpy type instead may make this less hacky, but looks like it would introduce ~500+MB which is quite large, so not ideal.

I think we don't want to do this in general to have a lightweight minimal container.

Tabrizian · 2022-06-24T17:30:34Z

IMO, this library looks very fragile. Is there any other alternative bfloat16 library that we could be using? What was the conclusion on using DLPack?

Forking/patching the bfloat16 package to simply fix the GreenWaves-Technologies/bfloat16#2 ourselves? Maybe it's worth spending some cycles to find the root cause and re-evaluate?

I think this is the problem with poorly maintained open-source libraries. The issue is open for 5 months and nobody has responded to it. I think it might be better to search for alternative solutions.

tanmayv25 · 2022-07-07T21:46:33Z

I think for bfloat16, or any other future new data type. We can add a capability in python client library to provide tensor contents as raw bytes.

add blfoat to utils

dbac3d6

jbkyang-nvi requested review from tanmayv25 and rmccorm4 June 17, 2022 20:51

jbkyang-nvi mentioned this pull request Jun 17, 2022

Add bfloat to client triton-inference-server/server#4521

Closed

jbkyang-nvi requested a review from GuanLuo June 17, 2022 20:53

rmccorm4 approved these changes Jun 21, 2022

View reviewed changes

Tabrizian reviewed Jun 21, 2022

View reviewed changes

rmccorm4 requested changes Jun 21, 2022

View reviewed changes

add workaround to avoid needing to import bfloat16

2974104

jbkyang-nvi force-pushed the kyang-python-bf16 branch from 9705f41 to 2974104 Compare June 22, 2022 00:43

rmccorm4 reviewed Jun 22, 2022

View reviewed changes

rmccorm4 requested changes Jun 22, 2022

View reviewed changes

jbkyang-nvi requested a review from rmccorm4 June 22, 2022 01:02

jbkyang-nvi added 2 commits June 21, 2022 18:07

add comment about incompatibility with tensorflow

827401d

addressed comments

fe561e2

rmccorm4 reviewed Jun 23, 2022

View reviewed changes

jbkyang-nvi requested a review from rmccorm4 June 23, 2022 22:43

addressed more comments

93c3db8

jbkyang-nvi force-pushed the kyang-python-bf16 branch from 47a969c to 93c3db8 Compare June 23, 2022 23:55

jbkyang-nvi closed this Jul 19, 2022

jbkyang-nvi deleted the kyang-python-bf16 branch July 19, 2022 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bfloat to client utils #118

Add bfloat to client utils #118

jbkyang-nvi commented Jun 17, 2022 •

edited

Loading

rmccorm4 Jun 21, 2022 •

edited

Loading

Tabrizian left a comment

rmccorm4 Jun 21, 2022

jbkyang-nvi commented Jun 22, 2022

rmccorm4 Jun 22, 2022

rmccorm4 Jun 22, 2022

jbkyang-nvi Jun 22, 2022

rmccorm4 Jun 22, 2022

jbkyang-nvi Jun 22, 2022 •

edited

Loading

rmccorm4 left a comment

rmccorm4 left a comment •

edited

Loading

jbkyang-nvi commented Jun 23, 2022 •

edited

Loading

Tabrizian commented Jun 24, 2022 •

edited

Loading

tanmayv25 commented Jul 7, 2022

		@@ -125,6 +125,7 @@ def debug_details(self):


		def np_to_triton_dtype(np_dtype):
		from bfloat16 import bfloat16

Add bfloat to client utils #118

Add bfloat to client utils #118

Conversation

jbkyang-nvi commented Jun 17, 2022 • edited Loading

rmccorm4 Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

Tabrizian left a comment

Choose a reason for hiding this comment

rmccorm4 Jun 21, 2022

Choose a reason for hiding this comment

jbkyang-nvi commented Jun 22, 2022

rmccorm4 Jun 22, 2022

Choose a reason for hiding this comment

rmccorm4 Jun 22, 2022

Choose a reason for hiding this comment

jbkyang-nvi Jun 22, 2022

Choose a reason for hiding this comment

rmccorm4 Jun 22, 2022

Choose a reason for hiding this comment

jbkyang-nvi Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

rmccorm4 left a comment

Choose a reason for hiding this comment

rmccorm4 left a comment • edited Loading

Choose a reason for hiding this comment

jbkyang-nvi commented Jun 23, 2022 • edited Loading

Tabrizian commented Jun 24, 2022 • edited Loading

tanmayv25 commented Jul 7, 2022

jbkyang-nvi commented Jun 17, 2022 •

edited

Loading

rmccorm4 Jun 21, 2022 •

edited

Loading

jbkyang-nvi Jun 22, 2022 •

edited

Loading

rmccorm4 left a comment •

edited

Loading

jbkyang-nvi commented Jun 23, 2022 •

edited

Loading

Tabrizian commented Jun 24, 2022 •

edited

Loading