Add manual eviction support

[tfpgh]

Adds config.reinsert_prob to have some probability of reinserting a sample into the buffer after being read. Set to 0.0 by default so it shouldn't affect existing code.

This could be made to work with fill_once but it's a lot uglier and I don't think anyone would use it.

[YassineYousfi]

idx_list is already random,
something like this should be enough

-     r.sadd(f'{queue_name}-empty', *idx_list)
+
+     keep = int(config.reinsert_prob * len(idx_list))
+     r.sadd(f'{queue_name}-empty', *idx_list[keep:])
+     if keep > 0: r.sadd('{queue_name}-full', *idx_list[:keep])

[tfpgh]

1. The randomness in idx_list comes from Redis SPOP. This guarantees the set of elements is random but the order they're returned in is often not random:

$ redis-cli
127.0.0.1:6379> SADD spop_test 1 2 3 4 5 6 7 8 9 10 11 12
(integer) 12
127.0.0.1:6379> SPOP spop_test 4
1) "7"
2) "8"
3) "9"
4) "11"
127.0.0.1:6379> SPOP spop_test 4
1) "3"
2) "6"
3) "10"
4) "12"

2. This makes the number of elements reinserted constant instead of a binomial distribution, not sure if this distinction matters.

[tfpgh]

@YassineYousfi I looked at tianshou, RLlib, and stable-baselines3 and you're generally right: replay happens on the training side and not in the dataloader.

I do still think putting reinsertion in gigashuffle is cleaner. The way we want to use it in rldriving is not specific to RL, it's a generic "reuse expensive samples" parameter. gigashuffle already introduces off-policy-ness with its buffer. Putting replay in training would stack another buffer which is harder to reason about in my opinion. It also bloats training code a lot more than it adds here.

[haraschax]

I agree it's cleaner in gigashuffle

[YassineYousfi]

I think the better way to do this is:

• add an option to not evict on reads DataloaderConfig
• make the dataloader return the indices in the Buffer objects
• expose an eviction method in MultiprocessShuffledDataloader

[haraschax]

Agreed, that leaves it to the trainer to choose an eviction strategy

[tfpgh]

This changes the interface, ty in xx will complain on existing code

[YassineYousfi]

I think it's reasonable to have Buffer always contain the indices.

[YassineYousfi]

lgtm besides always returning the indices (maybe as an attribute of the Buffer objects if that’s clean)

[tfpgh]

lgtm besides always returning the indices (maybe as an attribute of the Buffer objects if that’s clean)

I agree, this is cleaner and existing code shouldn't be affected (unless it iterates over the dictionary keys in some weird way).