Skip to content

Conversation

@fpetrini15
Copy link
Contributor

@fpetrini15 fpetrini15 commented Aug 16, 2022

Changes are made in conjunction with changes made to the third_party repo: triton-inference-server/third_party#21

jbkyang-nvi and others added 30 commits April 4, 2022 13:08
* Allow jetson test to confiure default shm size for python backend

* Update jetson docs for 22.03
* Add test case for shutdown

* Address comment

* Fix up

* Address comment
* Add metric API unit test to L0_metrics

* Fix up

* Fix shared library path
* Set address for metrics when http service is disable

* Update comment

* Update documentation
* Add testing for the lifetime of BLS tensors

* Review edit
* Add test for optional input in Python backend

* Review edit

* Review edit

* Reduce the default backend shared memory usage

* Reduce the tensor size of non-contiguous test
* Add Perf Analyzer unit tests CI test

* Addressed comments
* Add CLI test for MLFlow plugin

* Add Python test. Fix up

* Add newline
* Build pa with tfs and ts

* Update flag names

* Install cmake and rapidjson on sdk for test

* Add l0 client build variants

* Move installation of build deps to test

* Add indent

* Build without TFS/TS for most variants

* Remove cmake 18.04 version
- CPU only builds need deps from GPU enabled container
* Update build.py to generate scripts containing build commands

* Add --enable-all to build.py

* Update docs for new build.py behavior

* Don't include build dir in copyright check

* Format
* Fix compose.py to allow configurable gpu-min container

* Fix missing gpu-min in allowed image names
* updated qa tests to expect the new max_batch_size

default-max-batch-size is now a global backend config

documented default-max-batch-size feature

* parsing backend config options is now global backend aware
Small fix to render test documentation correctly.
* Fix L0_backend_python

* Review edit
build.py Outdated
Comment on lines 2308 to 2313
# apt get update
# apt install -y install default-jre
# apt install -y doctest-dev
# apt install -y libboost-program-options-dev
# LD_LIBRARY_PATH=/opt/tritonserver/lib:$LD_LIBRARY_PATH
# LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also removing "libboost-program-options-dev " from build.py. It used to be a dependency but after removing many of the unnecessary directories from the modern-cpp-kafka directory in the third_party repo, I don't think it's necessary anymore. Mentioning it just in case.

# Endpoints
option(TRITON_ENABLE_HTTP "Include HTTP API in server" ON)
option(TRITON_ENABLE_GRPC "Include GRPC API in server" ON)
option(TRITON_ENABLE_KAFKA "Include Kafka API in server" ON)
Copy link
Contributor

@rmccorm4 rmccorm4 Sep 9, 2022

Choose a reason for hiding this comment

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

should this be OFF by default? @GuanLuo

and if so, we might need to update some gitlab-ci.yaml or build.py arg to set it to ON explicitly for CI.

Comment on lines +79 to +88
# On windows the paths invoked by the script (running in WSL) must use
# /mnt/c when needed but the paths on the tritonserver command-line
# must be C:/ style.
if [[ "$(< /proc/sys/kernel/osrelease)" == *microsoft* ]]; then
MODELDIR=${MODELDIR:=C:/models}
DATADIR=${DATADIR:="/mnt/c/data/inferenceserver/${REPO_VERSION}"}
BACKEND_DIR=${BACKEND_DIR:=C:/tritonserver/backends}
SERVER=${SERVER:=/mnt/c/tritonserver/bin/tritonserver.exe}
export WSLENV=$WSLENV:TRITONSERVER_DELAY_SCHEDULER
Copy link
Contributor

Choose a reason for hiding this comment

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

is this test going to be run on windows?

auto_offset_reset='latest',
enable_auto_commit=True,
bootstrap_servers=f"{_tritonserver_ipaddr}:9092")
KafkaTest.consumer_.poll()
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment with link to what this "dummy poll" is for

# Brand new topic partition
print(end)
if(end != 0):
self.consumer_.seek(partition, end-1)
Copy link
Contributor

@rmccorm4 rmccorm4 Sep 9, 2022

Choose a reason for hiding this comment

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

plus link if relevant

Suggested change
self.consumer_.seek(partition, end-1)
# seek to latest message (end-1)
self.consumer_.seek(partition, end-1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mentioned in the comment block just above

// Permitted b/c strings are stored as contiguous memory in c++11
char* start = &binary_data[binary_data_offset];
// TODO: This should not be necessary. Retry using memcpy.
unsigned char* base = (unsigned char*)malloc(payload_length + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

use c++ style cast

DisplayConsumerTopics();

while (consumer_active_) {
auto records = consumer_->poll(std::chrono::milliseconds(100));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an event based system where consumer wakes up when there's a request, or does it have to be polled? Is the 100ms arbitrary or a best practice?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should add this to the doc

@manhtd98
Copy link

manhtd98 commented Oct 1, 2022

Did we got anything there? Kafka is nice idea for multi triton server

@pthalasta
Copy link

Just curious, are there any updates when this will be merged? Looking for kafka endpoint support as well!

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.