ray-project / ray

Ray is an AI compute engine. Ray consists of a core distributed runtime and a set of AI Libraries for accelerating ML workloads.
https://ray.io
Apache License 2.0
33.89k stars 5.76k forks source link

[Bug] Unable to subclass PPOTrainer #20970

Closed holinov closed 2 years ago

holinov commented 2 years ago

Search before asking

Ray Component

Ray Tune

What happened + What you expected to happen

Unable to subclass PPOTrainer. I'm trying to customize some logs and some logic for PPOTrainer and i get error when i try to subclass PPOTrainer

ray.exceptions.RayActorError: The actor died because of an error raised in its creation task, ray::TradingEnvTrainer.__init__() (pid=381565, ip=10.66.66.11)
  File "/root/w/analytics/lib/agents/ppo/trainers/trading_env_trainer.py", line 5, in __init__
    super(LoadablePPOTrainer, self).__init__(config, **kwargs)
  File "/opt/miniconda/lib/python3.8/site-packages/ray/rllib/agents/trainer_template.py", line 137, in __init__
    Trainer.__init__(self, config, env, logger_creator)
  File "/opt/miniconda/lib/python3.8/site-packages/ray/rllib/agents/trainer.py", line 623, in __init__
    super().__init__(config, logger_creator)
  File "/opt/miniconda/lib/python3.8/site-packages/ray/tune/trainable.py", line 107, in __init__
    self.setup(copy.deepcopy(self.config))
  File "/opt/miniconda/lib/python3.8/site-packages/ray/rllib/agents/trainer_template.py", line 147, in setup
    super().setup(config)
TypeError: super(type, obj): obj must be an instance or subtype of type

Versions / Dependencies

Reproduction script

import ray.rllib.agents.ppo as ppo

class TradingEnvTrainer(ppo.PPOTrainer):
    def __init__(self, config, **kwargs):
        super(TradingEnvTrainer, self).__init__(config, **kwargs)

tune.run(....)

Anything else

No response

Are you willing to submit a PR?

holinov commented 2 years ago

This could be reproduced using solution from https://github.com/ray-project/ray/issues/8379

krfricke commented 2 years ago

I can't reproduce the error - the following code works fine for me:

import ray.rllib.agents.ppo as ppo
from ray import tune

class TradingEnvTrainer(ppo.PPOTrainer):
    def __init__(self, config, **kwargs):
        super(TradingEnvTrainer, self).__init__(config, **kwargs)

tune.run(
    TradingEnvTrainer,
    config={
        "num_workers": 4,
        "env": "CartPole-v0"
    }
)

Are you somehow shadowing the self parameter?

krfricke commented 2 years ago

It seems we made some changes in this file recently, can you try installing the nightly wheels?

Though I just tested the code with Ray 1.9.0 on Python 3.8.7 and it still works for me.

gjoliver commented 2 years ago

yeah, I tested 1.9 as well, and couldn't reproduce it. if you have the exactly script, we can take another loo.

holinov commented 2 years ago

Yeph. I'v found point that breaks it. You need 2 files. First file: run.py

from ray import tune
import ray.rllib.agents.ppo as ppo

config = {
    "num_workers": 4,
    "env": "CartPole-v0"
}

class TradingEnvTrainer(ppo.PPOTrainer):
    def __init__(self, config, **kwargs):
        super(TradingEnvTrainer, self).__init__(config, **kwargs)

def start_trail(**kwargs):
    tune.run(
        TradingEnvTrainer,
        config=config
    )

if __name__ == "__main__":
    start_trail()

second file test.py

from run import *
config.update({
    'num_gpus': 0.5
})
start_trail()

If you launch run.py - everything is ok. But if you launch test.py - you get that error

gjoliver commented 2 years ago

I can reproduce this with 1.9 locally. let me take a look.

gjoliver commented 2 years ago

Fortunately, this has already been resolved in master. We realized during last quarter that build_trainer() is really confusing, and sometimes cause weird issues described by this issue. So we spent a lot of time and migrated all of our agents off build_trainer(). They are all simply extending Trainer class with latest HEAD.

You should get the change with next 1.10 release. If helpful, you can also use a nightly wheel for now to unblock you.

Thanks for the report.

gjoliver commented 2 years ago

@sven1977 for your information. that was a visionary change :)