Calling methods on pytorch Generator causes segfault

[sbrunk]

Something seems odd with the mapping of Generator. Calling seed and state related methods on a Generator instance triggers a segfault.

For instance running this snippet:

//> using lib "org.bytedeco:pytorch-platform:1.12.1-1.5.8"

val g = new org.bytedeco.pytorch.Generator()
g.current_seed()

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x000000016bd576d2, pid=14945, tid=9731
#
# JRE version: OpenJDK Runtime Environment Temurin-17+35 (17.0+35) (build 17+35)
# Java VM: OpenJDK 64-Bit Server VM Temurin-17+35 (17+35, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, bsd-amd64)
# Problematic frame:
# C  [libjnitorch.dylib+0x13b6d2]  Java_org_bytedeco_pytorch_Generator_current_1seed+0x42
...

Any ideas what could be wrong here?

[saudet]

That looks like an abstract class. Did you try to create an instance of a subclass instead?

[saudet]

It looks like the way to create a Generator is with make_generator(), so we should work on mapping that, I guess?
https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/test/cpu_rng_test.cpp

[sbrunk]

Thanks for having a look and apologies, I missed your first response.

In the meantime I also realized that functions like randperm() that take an optional generator also respect setting the global RNG state via manual_seed which unblocked me while writing deterministic tests. :)

But being able to call make_generator() to create Generator instances is still a good idea IMHO.

[sbrunk]

I'd like to get back into getting this in. generator.h is already in the presets and the make_generator function in C++ seems to be defined here:

https://github.com/pytorch/pytorch/blob/66a2600b6aadc5ee18ad4259f8a9f745dbaddc9a/aten/src/ATen/core/Generator.h#L143-L146

template<class Impl, class... Args>
Generator make_generator(Args&&... args) {
  return Generator(c10::make_intrusive<Impl>(std::forward<Args>(args)...));
}

Do we need additional mapping infos because it is a template function?

[HGuillemet]

Yes, we need explicit infos to instantiate the function template.
In the new version of the presets I'm working on, I'll add make_generator_cuda and make_generator_cpu to map at::make_generator<at::CUDAGeneratorImpl,int8_t> and at::make_generator<at::CPUGeneratorImpl,uint64_t> respectively.
Would that be enough ?

[sbrunk]

Yes that'd be great. In the Python impl, the Generator constructor takes a dtype and then decides which make_generator to call:

  if (device.type() == at::kCPU) {
    self->cdata = make_generator<CPUGeneratorImpl>();
  }
#ifdef USE_CUDA
  else if (device.type() == at::kCUDA) {
    self->cdata = make_generator<CUDAGeneratorImpl>(device.index());
  }
#elif USE_MPS
  else if (device.type() == at::kMPS) {
    self->cdata = make_generator<MPSGeneratorImpl>();
  }

So if we have the cpu and cuda variants (and mps in the future as well once we support building against mps), it should be straightforward to do it in a similar way.

Out of curiosity, is the new version of the presets you are working on a major overhaul?

[HGuillemet]

There will be 4 variants:

@Namespace("at") public static native @ByVal @Name("make_generator<at::CUDAGeneratorImpl>") Generator make_generator_cuda();
@Namespace("at") public static native @ByVal @Name("make_generator<at::CUDAGeneratorImpl,int8_t>") Generator make_generator_cuda(@Const byte device_index);
@Namespace("at") public static native @ByVal @Name("make_generator<at::CPUGeneratorImpl>") Generator make_generator_cpu();
@Namespace("at") public static native @ByVal @Name("make_generator<at::CPUGeneratorImpl,uint64_t>") Generator make_generator_cpu(long seed_in);

Out of curiosity, is the new version of the presets you are working on a major overhaul?

Yes. Hopefully coming soon as a PR.
Do you have any additional wishes for this version ?

[sbrunk]

Yes. Hopefully coming soon as a PR.

Looking forward to it.

Do you have any additional wishes for this version ?

None that I know of yet. Right now I'm just curious how this might affect the Scala bindings I'm working on.

[sbrunk]

With #1360 merged, we can now use the make_generator_cpu() and make_generator_cuda() variants.