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

Environmental variables for Jitify/RTC #253

Open
5 of 14 tasks
ptheywood opened this issue May 6, 2020 · 7 comments
Open
5 of 14 tasks

Environmental variables for Jitify/RTC #253

ptheywood opened this issue May 6, 2020 · 7 comments

Comments

@ptheywood
Copy link
Member

ptheywood commented May 6, 2020

Currently 2 environment variables are required to jitify/rtc compilation:

  • CUDA_PATH pointing to the root of the relevant CUDA version
  • FLAMEGPU2_INC_DIR pointint to flamegpu2_dev/include

This could be improved by:

  • also attempting other common environmental variables (CUDA_HOME/CUDA_ROOT are semi-common alternatives to CUDA_PATH)

  • fallback to trying the baked in path to the CUDA version used to compile the executable (if running on the same machine)

  • try he location of include from compile time (abs path, and / or relative to executable expected location)

  • Possibly pack the include headers into the executable? (this seems daft)

  • set FLAMEGPU2_ROOT instead, and infer /include from that.

  • Readme statment about CUDA_PATH being set by the installer is untrue for linux and also for windows when using the silent installer

This needs to be

  • Clearly documented

The current implementation (fbf2c8e) also doesn't check that the value of FLAMEGPU2_INC_DIR is valid or correct, leading to misleading errors, rather than an appropraite exception.

  • Check values in environment variables before using them, with appropriate exceptions if invalid
    • Check that FLAMEGPU2_INC_DIR is a valid directory, with atleast one flamegpu2 header file.
      • Ideally this should probably also do a version check once we have solid versioning built in, so that the "same" headers are used as the library was built with.
    • Check that CUDA_PATH is valid (look for bin/nvcc)
      • [ ] Ideally this should also check that the version of CUDA matches the version the library was compiled with? (Not sure if this is a hard requirement or not, or if it would work so long as the version is newer?)
      • Given the static library doesn't build any RTC related cuda code, does not need to be the same version, but we should probably check it is a supported CUDA version (which Cmake currently deals with for building the executables). I.e. check the version of NVCC on the CUDA_path is atleast 9.X (or in practice we can move to 10.0+?)

When including the FLAMEGPU2 repo via cmake in an individual example project, the include dir is not found automatically, requiring the env variable to be set. The env variable being set globally is also a potential issue, i.e. if you have 2 examples each using a different version of the core F2 on the same machine.

  what():  /home/ptheywood/code/flamegpu/FLAMEGPU2-circles-benchmark/build/_deps/flamegpu2-src/src/flamegpu/util/JitifyCache.cu(106): Error compiling runtime agent function: Unable to automatically determine include directory and FLAMEGPU2_INC_DIR environment variable does not exist, in JitifyCache::buildProgram().
  • Fix automatic search for the include directory for individual examples, when FLAMEGPU2_INC_DIR is not defined.
  • Figure out what to do for individual examples if FLAMEGPU2_INC_DIR is defined but is wrong. maybe fallback to checking locally?
  • Rename FLAMEGPU2_INC_DIR FLAMEGPU_INC_DIR, as it will not always be FLAMEGPU 2.x.y?
@Robadob
Copy link
Member

Robadob commented May 6, 2020

Possibly pack the include headers into the executable? (this seems daft)

It's certainly unusual, however current headers dir is 503kb, size of the smallest debug executable on windows is 20mb.

Bearing in mind, we plan to reduce the size of header dir further in future, it's not the most insane idea.

It's also the only portable method, assuming no weird machines without write access. Although jitify does just load the headers from disk, so we could pass them as string in the jitify (filename)\n<file data> format.

Robadob added a commit that referenced this issue Oct 14, 2020
* RTC will now attempt to auto find the include dir if variable is not set.
* CMake configure step auto generates a version file which is used for ensuring header version matches library build version.
(Might wish to make the version file generation behaviour different in future if this ends up too aggressive)
Partially addresses #253 
Co-authored-by: Peter Heywood <peethwd@gmail.com>
@ptheywood
Copy link
Member Author

The recent patch searches relative to the current working directory, rather than the current binary's directory, so calling from outside can lead to the path not being found.
Both would probably be a wise choice.

@Robadob
Copy link
Member

Robadob commented Oct 19, 2020

That's a good shout, I didn't consider it tbh. I'd just need to check each platform's way of getting binary location. Trivial addition.

https://stackoverflow.com/a/1528493/1646387

@ptheywood
Copy link
Member Author

Some of this should also be conisdered pre binary release #514

@Robadob
Copy link
Member

Robadob commented Jul 21, 2021

Worth noting, the Python wheel now includes the RTC headers. Though C RTC binaries don't.

@ptheywood
Copy link
Member Author

support FLAMEGPU2_INC_DIR (deprecated but works, lwoer priotiy) and FLAMEGPU_INC_DIR

ptheywood added a commit that referenced this issue Aug 3, 2021
Checks for CUDA_PATH and then CUDA_HOME to find the cuda include directory.
Checks for FLAMEGPU_INC_DIR and then FLAMEGPU2_INC_DIR for the flamegpu include direcotry

Part of #253
ptheywood added a commit that referenced this issue Aug 6, 2021
Checks for CUDA_PATH and then CUDA_HOME to find the cuda include directory.
Checks for FLAMEGPU_INC_DIR and then FLAMEGPU2_INC_DIR for the flamegpu include direcotry

Part of #253
@Robadob
Copy link
Member

Robadob commented Jul 5, 2023

#1074 Adds GLM headers to Python wheels (as this seems compliant with the license as that's packaged too) and setups up GLM_INC_DIR environment variable to override path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants