[BUG] Cython memory resource classes duplicate C++ class default parameters

[harrism]

Describe the bug
RMM's Cython MemoryResource classes such as PoolMemoryResource should not duplicate (and hard code) default constructor parameters that are also defined at a lower level in C++ device_memory_resource classes.

cdef class FixedSizeMemoryResource(MemoryResource):
    def __cinit__(
            self,
            MemoryResource upstream,
            size_t block_size=1<<20,
            size_t blocks_to_preallocate=128
    ):
        self.c_obj.reset(
            new fixed_size_memory_resource_wrapper(
                upstream.c_obj,
                block_size,
                blocks_to_preallocate
            )
        )

    def __init__(
            self,
            MemoryResource upstream,
            size_t block_size=1<<20,
            size_t blocks_to_preallocate=128
    ):

These hard-coded values duplicate the C++:

// A block is the fixed size this resource alloates
  static constexpr std::size_t default_block_size = 1 << 20;  // 1 MiB
  // This is the number of blocks that the pool starts out with, and also the number of
  // blocks by which the pool grows when all of its current blocks are allocated
  static constexpr std::size_t default_blocks_to_preallocate = 128;
  // The required alignment of this allocator
  static constexpr std::size_t allocation_alignment = 256;

  /**
   * @brief Construct a new `fixed_size_memory_resource` that allocates memory from
   * `upstream_resource`.
   *
   * When the pool of blocks is all allocated, grows the pool by allocating
   * `blocks_to_preallocate` more blocks from `upstream_mr`.
   *
   * @param upstream_mr The memory_resource from which to allocate blocks for the pool.
   * @param block_size The size of blocks to allocate.
   * @param blocks_to_preallocate The number of blocks to allocate to initialize the pool.
   */
  explicit fixed_size_memory_resource(
    Upstream* upstream_mr,
    std::size_t block_size            = default_block_size,
    std::size_t blocks_to_preallocate = default_blocks_to_preallocate)
    : upstream_mr_{upstream_mr},
      block_size_{rmm::detail::align_up(block_size, allocation_alignment)},
      upstream_chunk_size_{block_size * blocks_to_preallocate}
  {
    // allocate initial blocks and insert into free list
    this->insert_blocks(std::move(blocks_from_upstream(cudaStreamLegacy)), cudaStreamLegacy);
  }

This duplication means two places to make changes, and can lead to bugs.

[shwina]

I'll look into this, but it looks there will have to be some redundancy, as we have to expose the actual default values of these parameters to Python users (either in the documentation or in the function signature)

[harrism]

You don't need to expose sentinel values like ~0 to users. You can see how I changed this.

[shwina]

Yup - agreed that for the sentinel values like ~0, we should just expose None instead.

What should we do about concrete values like the default block size of 1 MiB, or blocks_to_preallocate=128? That's valuable information that we shouldn't hide from users.

[jrhemstad]

What should we do about concrete values like the default block size of 1 MiB, or blocks_to_preallocate=128? That's valuable information that we shouldn't hide from users.

Default values shouldn't be duplicated because they can always change without warning. Cython/Python are free to define their own default values that get passed explicitly to the C++ ctors.

[shwina]

Default values shouldn't be duplicated because they can always change without warning. Cython/Python are free to define their own default values that get passed explicitly to the C++ ctors.

In this case, that's what we're doing.

[harrism]

I think we should define constants for the C++ side defaults that can be queried and used on the Python side, just like I did for the sentinels. Then the Python API doesn't need to pick its own (potentially bad) values, and when we decide there are better values to use for the C++ side they will propagate.

[jakirkham]

That sounds like a good idea to me 🙂

[github-actions]

This issue has been marked rotten due to no recent activity in the past 90d. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

[github-actions]

This issue has been marked stale due to no recent activity in the past 30d. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be marked rotten if there is no activity in the next 60d.

[shwina]

I think with #661, this will also be closed out.

[harrism]

Still relevant and being worked in #662

[kkraus14]

Fixed by #662