search

triton-inference-server / fil_backend

FIL backend for the Triton Inference Server
Apache License 2.0
72 stars 36 forks source link

Apply changes to CMake configuration #373

Closed mc-nv closed 10 months ago

wphicks commented 10 months ago

@mc-nv, I'm assuming that this is a follow up to #371 because it did not solve your problem? I'm fine with the cmake minimum version change, but can you help me understand why the project declaration needs to be moved outside of the if/else branch as we discussed on #371?

mc-nv commented 10 months ago

@mc-nv, I'm assuming that this is a follow up to #371 because it did not solve your problem? I'm fine with the cmake minimum version change, but can you help me understand why the project declaration needs to be moved outside of the if/else branch as we discussed on #371?

Previous commit has a CMakeLists.txt constraint where minimal required version must be declared as top-level instruction.

Note Call the cmake_minimum_required() command at the beginning of the top-level CMakeLists.txt file even before calling the project() command. It is important to establish version and policy settings before invoking other commands whose behavior they may affect. See also policy CMP0000.

Regarding removing project() command, I simply follow the book approach where according to enable_language() command documentation this instruction is more elegant IMO.

Enables support for the named languages in CMake. This is the same as the project() command but does not create any of the extra variables that are created by the project command.

wphicks commented 10 months ago

Previous commit has a CMakeLists.txt constraint where minimal required version must be declared as top-level instruction.

The real issue here is that our CMakeLists.txt is really two CMakeLists.txt files stacked on top of one another for convenience. It would be useful not to have to constrain the CMake version to something so recent if we're just going to invoke the build through Docker, but once we're working with the actual bare-metal build, we need 3.26.4. The guidance on when to call cmake_minimum_required is generally correct, but we should interpret that as calling it at the top of each branch of the if/else.

Practically, this is a concern because RAPIDS is almost always at one of the most recent CMake versions, but it would be preferable not to always impose that condition on Triton.

As for the project declaration, I'm not too opinionated on it either way. I slightly prefer the separate project declarations because it emphasizes the above idea that these are really two different build configs, but enable_language works just fine too.

mc-nv commented 10 months ago

@wphicks we are happy with any solution that will solve compilation process for Triton.

We can come back to https://github.com/triton-inference-server/fil_backend/pull/372 changes if that's what you willing to see.

If so please feel free to close current PR and reopen previous or create new one.

wphicks commented 10 months ago

we are happy with any solution that will solve compilation process for Triton

Of course! That's number one priority, and I'm happy to get a temporary solution in and then clean up if necessary with further discussion. Could you file an issue with a reproducer for the problem you're seeing so we can track both the initial solution and any necessary follow-up? I want to make sure that any subsequent clean-up we do does not re-introduce the problem, and I'm not entirely sure what the problem is at the moment.

wphicks commented 10 months ago

Sorry, @mc-nv, forgot to tag you in the above.

Tabrizian commented 10 months ago

@wphicks Sorry, it was probably my fault for putting the previous PR for review before making sure it fixed the issue in our CI (the original version did, but after applying reviews it didn't). All we need is just a minimum CMake version in the docker build branch to get the build passing.

Tabrizian commented 10 months ago

Looks like the issue is that "VERSION" in project is not supported in all CMake versions and that's the error we're currently observing in the CI.

wphicks commented 10 months ago

@Tabrizian Great! Thanks very much for the context. Could you go ahead and post an issue with reproducer now? I'm 100% okay with merging a fix that just gets CI green again, but I want to make sure I understand the issue. CMake's project command has supported VERSION since 3.0.2 at least (and maybe earlier), so after we get CI green, I'd like to investigate further.

For now, I can't confirm that this change gets CI green, since build.py builds are blocked by the current problem with fetching boost. Our other build path does not reproduce the issue.

Tabrizian commented 10 months ago

Let's get this merged: https://github.com/triton-inference-server/fil_backend/pull/372

Green pipeline: https://gitlab-master.nvidia.com/dl/dgx/tritonserver/-/jobs/78848045

I'm going to file a reproducer for this issue to make sure it is covered in CI for future changes.

wphicks commented 10 months ago

Accidentally merged this one, but reverted and merged #372. Thanks, @Tabrizian!