Combine Dataloader and Dataset class into EnvironmentInterface

Monty Code
7

Hello Viviane and Tristan,

I’ve hit a bit of a roadblock, plus have two questions about how I’m approaching this task as I work through it. Here’s the draft PR (still a WIP - I’m on step two of Tristan’s outlined steps here).

Roadblock: I’m encountering segfaults when running unit tests due to the way the environment dataloader is now managing deallocation of the simulator. It seems multiple places are attempting to close the environment dataloader, so we end up with the following log output:

DEBUG:tbp.monty:close:623:Removing and closing python log handler: <FileHandler /var/folders/7p/p342_4c902x8py77cp4k46640000gn/T/tmpuyxd2nol/log.txt (NOTSET)>
[01:28:02:137462]:[Physics] BulletPhysicsManager.cpp(35)::~BulletPhysicsManager : 
Deconstructing BulletPhysicsManager
GL::Context::current(): no current contextFatal Python error: Aborted

Current thread 0x00000002028cb200 (most recent call first):
File “miniconda3/envs/tbp.monty/lib/python3.8/site-packages/habitat_sim/simulator.py”, line 152 in close
File “src/tbp/monty/simulators/habitat/simulator.py”, line 631 in close
File “src/tbp/monty/simulators/habitat/environment.py”, line 178 in close
File “src/tbp/monty/frameworks/environments/embodied_environment.py”, line 119 in _del_
File “/Users/annarussokennedy/miniconda3/envs/tbp.monty/lib/python3.8/unittest/case.py”, line 633 in _callTestMethod

I’ve tried removing all but one of the close() / __exit__() / __del__() calls up that call chain (including the one in EnvironmentDataloader itself), and tried stepping through with python’s debugger (as well as lldb, given the issue is in the underlying C++ code), but am missing too much context on the interplay of the classes in the environment/simulator/embodied_environment and habitat sim files, so haven’t yet gotten to the bottom of it. I thought I should pause and check in here to verify my approach before going further.

If you have any suggestions for getting to the bottom of this, or directions for how you’d ideally like to see these resources allocated/deallocated centrally, I’d be grateful for tips.

Questions

Given the following class hierarchy, defined in embodied_data.py:

class EnvironmentDataLoader:…
class EnvironmentDataLoaderPerObject(EnvironmentDataLoader):…
class InformedEnvironmentDataLoader(EnvironmentDataLoaderPerObject):…
class OmniglotDataLoader(EnvironmentDataLoaderPerObject):…
class SaccadeOnImageDataLoader(EnvironmentDataLoaderPerObject):…
class SaccadeOnImageFromStreamDataLoader(SaccadeOnImageDataLoader):…

I’ve refactored the child classes to call their super class __init__ methods, since the EnvironmentDataset that was previously required as an initialization param for those child classes is now integrated into EnvironmentDataLoader, the super class. However there are a few parts that aren’t straightforward with the refactoring.

Question 1: OmniglotDataLoader’s super class EnvironmentDataLoaderPerObject expects object_names and object_init_sampler as init params, and calls create_semantic_mapping based on those. OmniglotDataLoader needs the environment to be initialized in order to define its object_names list -

object_names = [
    str(self.env.alphabet_names[alphabets[i]])
    + “_”
    + str(self.characters[i])
    for i in range(n_objects)
]

but the environment isn’t initiated until its super class runs. I handled this by letting object_init_sampler and object_names be optional params to EnvironmentDataLoaderPerObject, so that the child class can decide what to do with them. This also means create_semantic_mapping is not called if object_names is not provided. Does this approach align with how you’d like this handled?

Question 2: In order to update SaccadeOnImageFromStreamDataLoader, child class of SaccadeOnImageDataLoader, with these changes, I had to do something about the scenes and versions params that SaccadeOnImageDataLoader expects on __init__. I chose to make them optional on the super class, but again wanted to check whether this approach of using optional parameters in this way fits the conventions of this codebase?

If it’s easier to jump on a call to discuss any of the above, I’m happy to do that.

Thank you!

1 Like