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

Adding pyflamegpu #24483

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Adding pyflamegpu #24483

wants to merge 6 commits into from

Conversation

Robadob
Copy link

@Robadob Robadob commented Nov 13, 2023

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/pyflamegpu, recipes/pyflamegpu-no_seatbelts, recipes/pyflamegpu-visualisation, recipes/pyflamegpu-visualisation-no_seatbelts) and found some lint.

Here's what I've got...

For recipes/pyflamegpu:

  • The top level meta keys are in an unexpected order. Expecting ['package', 'source', 'build', 'requirements', 'test', 'about', 'extra'].
  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [17, 18, 19, 44, 45, 46, 47, 48, 50, 51, 52, 53]
  • The recipe must have a build/number section.
  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).
  • There are too few lines. There should be one empty line at the end of the file.

For recipes/pyflamegpu:

  • The pypa build package has been renamed to python-build.

For recipes/pyflamegpu-no_seatbelts:

  • The top level meta keys are in an unexpected order. Expecting ['package', 'source', 'build', 'requirements', 'test', 'about', 'extra'].
  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [17, 18, 19, 47, 48, 49, 50, 51, 53, 54, 55, 56]
  • The recipe must have a build/number section.
  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).
  • There are too few lines. There should be one empty line at the end of the file.

For recipes/pyflamegpu-no_seatbelts:

  • The pypa build package has been renamed to python-build.

For recipes/pyflamegpu-visualisation:

  • The top level meta keys are in an unexpected order. Expecting ['package', 'source', 'build', 'requirements', 'test', 'about', 'extra'].
  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [18, 19, 20, 21, 22, 39, 56, 57, 58, 59, 60, 62, 63, 64, 65]
  • The recipe must have a build/number section.
  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).
  • There are too few lines. There should be one empty line at the end of the file.

For recipes/pyflamegpu-visualisation:

  • The pypa build package has been renamed to python-build.

For recipes/pyflamegpu-visualisation-no_seatbelts:

  • The top level meta keys are in an unexpected order. Expecting ['package', 'source', 'build', 'requirements', 'test', 'about', 'extra'].
  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [18, 19, 20, 21, 22, 39, 57, 58, 59, 60, 61, 63, 64, 65, 66]
  • The recipe must have a build/number section.
  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).
  • There are too few lines. There should be one empty line at the end of the file.

For recipes/pyflamegpu-visualisation-no_seatbelts:

  • The pypa build package has been renamed to python-build.

Not yet addressed sha, as development
@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/pyflamegpu, recipes/pyflamegpu-no_seatbelts, recipes/pyflamegpu-visualisation, recipes/pyflamegpu-visualisation-no_seatbelts) and found some lint.

Here's what I've got...

For recipes/pyflamegpu:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).

For recipes/pyflamegpu:

  • The pypa build package has been renamed to python-build.

For recipes/pyflamegpu-no_seatbelts:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).

For recipes/pyflamegpu-no_seatbelts:

  • The pypa build package has been renamed to python-build.

For recipes/pyflamegpu-visualisation:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).

For recipes/pyflamegpu-visualisation:

  • The pypa build package has been renamed to python-build.

For recipes/pyflamegpu-visualisation-no_seatbelts:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).

For recipes/pyflamegpu-visualisation-no_seatbelts:

  • The pypa build package has been renamed to python-build.

@Robadob
Copy link
Author

Robadob commented Nov 13, 2023

@conda-forge-admin, please ping conda-forge/staged-recipes

Questions primarily relate to CUDA and build variants/metapackages, this is a Python/C++ hybrid package that uses CUDA.

I've been working through creating recipes, and have run into a few questions (although some are more general). I don't believe any of these are answered in the knowledge base

  1. Our package has four build variants (2x2), each independently compiled from source, I've named them in what I believe to be a meta-package style but would like clarity on whether this is the correct process. I can't find any particularly clear documentation on formatting build variants.

  2. Our package depends on CUDA, to get the local build to succeed I have currently been using the linux64 config. Using linux64_cuda112 prevents us from building with CUDA 12.0. Ideally we would like to have both CUDA 11.2 and CUDA 12.0 build variants for compatibility reasons, however CUDA 11.2 requires the cudatoolkit package and CUDA 12.0 has split CUDA packages. It's not clear how we can detect this to filter packages within meta.yml.

  3. The above linked "creating recipes" states:

In case your project has tests included, you need to decide if these tests should be executed while building the conda-forge feedstock.

Our package relies on CUDA, and hence tests specified in meta.yml won't be able to execute on CI runners. It's not clear how the execution of tests on conda-forge should be disabled.

  1. Our Windows builds are currently failing to detect CUDA support at CMake configure time on conda-forge CI. I tried alot of configurations before I found a combination that works with local vanilla conda build, and don't immediately recognise what would be the problem on your CI (I have added cuda-version >=12.0 to meta.yml since then). Knowledge base references __cuda, but I think that's related to run constraints, so shouldn't interfere with build.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/staged-recipes and so here I am doing that.

Before this change, tested that package fully builds on Linux with tests commented out.
@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/pyflamegpu, recipes/pyflamegpu-no_seatbelts, recipes/pyflamegpu-visualisation, recipes/pyflamegpu-visualisation-no_seatbelts) and found some lint.

Here's what I've got...

For recipes/pyflamegpu:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).

For recipes/pyflamegpu-no_seatbelts:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).

For recipes/pyflamegpu-visualisation:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).

For recipes/pyflamegpu-visualisation-no_seatbelts:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/staged-recipes and so here I am doing that.

1 similar comment
@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/staged-recipes and so here I am doing that.

@mondus
Copy link

mondus commented Nov 17, 2023

I agree to be a maintainer.

@carterbox
Copy link
Member

carterbox commented Apr 4, 2024

  1. Our package has four build variants (2x2), each independently compiled from source, I've named them in what I believe to be a meta-package style but would like clarity on whether this is the correct process. I can't find any particularly clear documentation on formatting build variants.

You can optionally add keywords to the build string of each build variant in order to make it easier to see which variant is installed in an environment. i.e. pytorch-2.1.2-cuda120_py39h832322d_303.conda. However, this is not strictly necessary because each variant will have a unique build string which is generated from hashing the dependencies used to build the package.

  1. Our package depends on CUDA, to get the local build to succeed I have currently been using the linux64 config. Using linux64_cuda112 prevents us from building with CUDA 12.0. Ideally we would like to have both CUDA 11.2 and CUDA 12.0 build variants for compatibility reasons, however CUDA 11.2 requires the cudatoolkit package and CUDA 12.0 has split CUDA packages. It's not clear how we can detect this to filter packages within meta.yml.

Our infrastructure creates separate build jobs for each CUDA version. We have a tool called conda-smithy which parses recipes and decided which build variants to automatically insert into the build matrix. The same process is used for building for multiple python versions.

  1. The above linked "creating recipes" states:

In case your project has tests included, you need to decide if these tests should be executed while building the conda-forge feedstock.

Our package relies on CUDA, and hence tests specified in meta.yml won't be able to execute on CI runners. It's not clear how the execution of tests on conda-forge should be disabled.

We do not offer build runners with GPUs, so you need to skip any tests that require loading the CUDA driver.

  1. Our Windows builds are currently failing to detect CUDA support at CMake configure time on conda-forge CI. I tried alot of configurations before I found a combination that works with local vanilla conda build, and don't immediately recognise what would be the problem on your CI (I have added cuda-version >=12.0 to meta.yml since then). Knowledge base references __cuda, but I think that's related to run constraints, so shouldn't interfere with build.

The build runners do not have any GPUs. Ensure that you are using our CMAKE_ARGS environment variable and the compilers that are set to the environment variables CC, CXX, etc. Also, only the LINUX runner is setup for CUDA here in staged recipes. There is no runner for Windows + CUDA until your recipe is given it's own feedstock.

@carterbox
Copy link
Member

carterbox commented Apr 4, 2024

Here in staged recipes, the package will only be built against CUDA 11.2 on linux. Once in the feedstock, it will be rerendered and then it will be built against 11.2, 11.8, and 12.x.

Here are some examples that are similar to your recipes:

#23415 -> https://github.com/conda-forge/carterbox-torch-radon-feedstock

https://github.com/conda-forge/libmagma-feedstock

Comment on lines +1 to +19
# https://docs.conda.io/projects/conda-build/en/3.21.x/resources/compiler-tools.html#using-your-customized-compiler-package-with-conda-build-3
# https://github.com/rapidsai/cuml/blob/branch-23.12/conda/recipes/libcuml/conda_build_config.yaml
# https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/main/recipe/conda_build_config.yaml
cuda_compiler:
- cuda-nvcc # requires 'conda build -c nvidia conda' on Windows
#- nvcc # requires 'conda build -c nvidia conda' on Windows
cuda_compiler_version:
#- 11.2 # [(linux or win64) and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
- 12.0 # [(linux or win64) and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
- 12.0
cuda_compiler_version_min:
#- 11.2 # [linux or win64]
- 12.0 # [linux or win64]
- 12.0

c_compiler: # [win]
- vs2022 # [win]
cxx_compiler: # [win]
- vs2022 # [win]
Copy link
Member

Choose a reason for hiding this comment

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

Delete this file. CUDA and compiler variants are managed by our infrastructure.

Comment on lines +19 to +24
missing_dso_whitelist:
- $RPATH/libcuda.so.1 # [linux] Ignore as this comes from CUDA driver
- $RPATH/libnvrtc.so.12 # [linux] Conda-build can't find this in run reqs cuda-cudart
- $RPATH/libcudart.so.12 # [linux] Conda-build can't find this in run reqs cuda-nvrtc
- $RPATH/nvrtc64_120_0.dll # [win64] Conda-build can't find this in run reqs cuda-cudart
- $RPATH/cudart64_12.dll # [win64] Conda-build can't find this in run reqs cuda-nvrtc
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
missing_dso_whitelist:
- $RPATH/libcuda.so.1 # [linux] Ignore as this comes from CUDA driver
- $RPATH/libnvrtc.so.12 # [linux] Conda-build can't find this in run reqs cuda-cudart
- $RPATH/libcudart.so.12 # [linux] Conda-build can't find this in run reqs cuda-nvrtc
- $RPATH/nvrtc64_120_0.dll # [win64] Conda-build can't find this in run reqs cuda-cudart
- $RPATH/cudart64_12.dll # [win64] Conda-build can't find this in run reqs cuda-nvrtc

These should not be needed because in staged-recipes there is only one CUDA variant available: CUDA 11.2.

Comment on lines +14 to +16
build:
number: 0
script_env:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
build:
number: 0
script_env:
build:
number: 0
skip: true # [cuda_compiler_version == "None"]
skip: true # [py < 310]
script_env:

Skip building the default non-cuda variant forever. Skip lower python versions for faster debugging for now, then unskip later.

Comment on lines +25 to +29
ignore_run_exports:
- python # [linux] Not clear why conda thinks this isn't used
- astpretty # [linux, win64] Not clear why conda thinks this isn't used
- vc14_runtime # [win64] Not clear why conda thinks this isn't used (we aren't static linking)
- ucrt # [win64] Not clear why conda thinks this isn't used (we aren't static linking)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ignore_run_exports:
- python # [linux] Not clear why conda thinks this isn't used
- astpretty # [linux, win64] Not clear why conda thinks this isn't used
- vc14_runtime # [win64] Not clear why conda thinks this isn't used (we aren't static linking)
- ucrt # [win64] Not clear why conda thinks this isn't used (we aren't static linking)

This is not the purpose of ignore_run_exports. You should only add host packages here that you know are NOT being used at runtime. e.g. You are using a C library that has some header-only APIs.

Comment on lines +35 to +37
- cuda-cudart-dev # [linux]
- cuda-nvrtc-dev # [linux]
- libcurand-dev # [linux]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- cuda-cudart-dev # [linux]
- cuda-nvrtc-dev # [linux]
- libcurand-dev # [linux]
{% if cuda_major == 12 %}
- cuda-cudart-dev # [linux]
- cuda-nvrtc-dev # [linux]
- libcurand-dev # [linux]
{% endif %}

These packages are only for CUDA >= 12. Our package structure changed for CUDA 12. Before that, everything was shipped as one monolithic cudatoolkit package.

Also, these are libraries, so they belong in the HOST requirements section.

- wheel
- python-build
run:
- cuda-version >=12.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- cuda-version >=12.0

The cuda version is run_exported by our cuda compiler package.

license_family: MIT
license_file: LICENSE.md
doc_url: https://docs.flamegpu.com/
dev_url: https://github.com/FLAMEGPU/FLAMEGPU2
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there are separate CXX and Python APIs? Can you build the CXX libraries first, then link the Python modules to them dynamically?

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

Successfully merging this pull request may close these issues.

None yet

3 participants