fhswf / MLPro

MLPro - The Integrative Middleware Framework for Standardized Machine Learning in Python
https://mlpro.readthedocs.io/
Apache License 2.0
12 stars 3 forks source link

Refact: New Repo for MuJoCo #939

Closed detlefarend closed 4 months ago

detlefarend commented 5 months ago

Motivation The wrapper code and related howto files of 3rd party package MuJoCo shall be relocated to a new repository to prevent MLPro from incompatible changes.

Benefit

Are the proposed changes backward compatible?

To do

Branch wrappers/relocation2

See also https://github.com/fhswf/MLPro/wiki/30-MLPro-Extension

steveyuwono commented 4 months ago

Hi @detlefarend, just reviewed this, I have a question regarding the integration. It is clear that the mlpro.wrappers.mujoco codes are going to be moved into the new repository. However, in the mlpro.bf.systems module, we have a System class that can be configured to work with a Mujoco file. In such cases, we need to initialize the wrapper if it is called, as shown in this link Then, this also affects the EnvBase class in _mlpro.rl.modelsenv. Should these integrations also be removed from MLPro, and updated System and EnvBase classes be created within the wrapper?

detlefarend commented 4 months ago

Hi @detlefarend, just reviewed this, I have a question regarding the integration. It is clear that the mlpro.wrappers.mujoco codes are going to be moved into the new repository. However, in the mlpro.bf.systems module, we have a System class that can be configured to work with a Mujoco file. In such cases, we need to initialize the wrapper if it is called, as shown in this link Then, this also affects the EnvBase class in _mlpro.rl.modelsenv. Should these integrations also be removed from MLPro, and updated System and EnvBase classes be created within the wrapper?

Hi @steveyuwono, I also checked the code. The design doesn't follow our usual wrapper concept, but it encapsulates the optional MuJoCo parts suitable. It should be enough to redirect the wrapper access in ll 1169, 1259 to mlpro_int_mujoco. The only idea I have to improve something is to embed the imports in a try/except statement with an error log that points out the need for installing our MuJoCo extension.

steveyuwono commented 4 months ago

Hi @detlefarend, just reviewed this, I have a question regarding the integration. It is clear that the mlpro.wrappers.mujoco codes are going to be moved into the new repository. However, in the mlpro.bf.systems module, we have a System class that can be configured to work with a Mujoco file. In such cases, we need to initialize the wrapper if it is called, as shown in this link Then, this also affects the EnvBase class in _mlpro.rl.modelsenv. Should these integrations also be removed from MLPro, and updated System and EnvBase classes be created within the wrapper?

Hi @steveyuwono, I also checked the code. The design doesn't follow our usual wrapper concept, but it encapsulates the optional MuJoCo parts suitable. It should be enough to redirect the wrapper access in ll 1169, 1259 to mlpro_int_mujoco. The only idea I have to improve something is to embed the imports in a try/except statement with an error log that points out the need for installing our MuJoCo extension.

Hi @detlefarend , okay, well noted. Thank you for the information.