Skip to content

Conversation

@pseudo-rnd-thoughts
Copy link

@pseudo-rnd-thoughts pseudo-rnd-thoughts commented Jan 7, 2026

I've been integration PufferDrive with RLib and ran into these problems.

  1. The INI file is relative, meaning that for projects where you pip install pufferdrive outside your current working directory, when loading PufferDrive this fails. I've added code within the bindings to help debug this for future users
  2. RLlib uses Gymnasium 1.x where as puffer uses 0.x, I don't see a reason in the code and haven't run into any issues so far with changing the version number
  3. The observation space I found did not contain all the observations which raised an error within PufferDrive about this

Also, I've made this PR for 3.0 rather than 2.0, is that correct?

@greptile-apps
Copy link

greptile-apps bot commented Jan 7, 2026

Greptile Summary

This PR addresses two issues encountered when integrating PufferDrive with RLlib: fixing the relative INI file path problem and updating Gymnasium to version 1.x for compatibility.

Key Changes

  • INI file path fix (drive.py): Replaced hardcoded relative path "pufferlib/config/ocean/drive.ini" with absolute path computed from package directory, solving issues when pufferdrive is pip installed outside the current working directory
  • Error handling improvements (binding.c): Added explicit file existence check and improved error messages with FileNotFoundError and ValueError exceptions instead of silent failures
  • Gymnasium version update (setup.py): Updated from 0.29.1 to 1.2.2 for RLlib compatibility
  • Numpy version constraint removal (setup.py): Removed numpy<2.0 constraint, now accepts any numpy version
  • Observation space bounds update (drive.py): Changed observation space upper bound from high=1 to high=2 (not mentioned in PR description)

Notes

The observation space bounds change from high=1 to high=2 on line 88 of drive.py is not explained in the PR description. Based on the observation computation code, goal positions are normalized with factor 0.005 (rel_goal_x * 0.005f), meaning goals up to 400 units away could exceed 1.0. This change appears intentional to accommodate actual observation ranges, but should be documented.

Confidence Score: 4/5

  • This PR is safe to merge with minor documentation needs
  • The changes are well-implemented and solve real integration issues. The INI path fix is robust, error handling is improved, and dependency updates are reasonable. Score reduced by 1 because the observation space bounds change wasn't documented in the PR description and should be verified/explained by the author
  • No files require special attention - all changes are straightforward

Important Files Changed

Filename Overview
pufferlib/ocean/drive/binding.c Added better error handling for INI file loading with informative error messages
pufferlib/ocean/drive/drive.py Fixed INI file path to be absolute and updated observation space bounds from high=1 to high=2
setup.py Updated Gymnasium from 0.29.1 to 1.2.2 and removed numpy version constraint

Sequence Diagram

sequenceDiagram
    participant User as User/RLlib
    participant Drive as Drive.__init__()
    participant Binding as binding C Module
    participant INI as INI File
    
    User->>Drive: Initialize environment
    Note over Drive: Compute _PACKAGE_DIR from __file__
    Note over Drive: Construct _INI_FILE absolute path
    
    Drive->>Binding: Create environment with ini_file=_INI_FILE
    Binding->>INI: fopen(ini_file, "r")
    
    alt File Not Found
        INI-->>Binding: NULL
        Binding-->>Drive: PyErr_SetString(FileNotFoundError)
        Drive-->>User: Exception raised
    else File Found
        INI-->>Binding: File handle
        Binding->>Binding: fclose(file handle)
        Binding->>INI: ini_parse(ini_file)
        
        alt Parse Failure
            INI-->>Binding: Error code < 0
            Binding-->>Drive: PyErr_SetString(ValueError)
            Drive-->>User: Exception raised
        else Parse Success
            INI-->>Binding: Configuration loaded
            Binding-->>Drive: Environment initialized
            Drive-->>User: Ready to use
        end
    end
Loading

@greptile-apps
Copy link

greptile-apps bot commented Jan 7, 2026

Greptile found no issues!

From now on, if a review finishes and we haven't found any issues, we will not post anything, but you can confirm that we reviewed your changes in the status check section.

This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR".

@NellyWhads
Copy link

If we could have this back-ported to the 2.0 release as well, it would be very useful!

@pseudo-rnd-thoughts
Copy link
Author

@NellyWhads Would you like me to make the same PR but for the 2.0 branch?

@pseudo-rnd-thoughts pseudo-rnd-thoughts changed the title Integration with RLlib Integration with RLlib (v3.0) Jan 7, 2026
@NellyWhads
Copy link

yes please, good sir!

@pseudo-rnd-thoughts
Copy link
Author

@NellyWhads Done in #250

What do I need to do for merging?

@eugenevinitsky
Copy link

Wow, thanks, just saw it! We'll assign a reviewer to it tomorrow

@pseudo-rnd-thoughts
Copy link
Author

Thanks @eugenevinitsky

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants