Implements set_env_attr and get_env_attr for vector environments

[Markus28]

• I have marked all applicable categories:
  • exception-raising fix
  • algorithm implementation fix
  • documentation modification
  • new feature
• I have reformatted the code using make format (required)
• I have checked the code using make commit-checks (required)
• If applicable, I have mentioned the relevant/related issue(s)
• If applicable, I have listed every items in this Pull Request below

This pull request aims to implement the features outlined in issue #473

• Implement get_env_attr, set_env_attr for each worker class
• Implement get_env_attr, set_env_attr for vector environments
• Corresponding unit tests and documentation

[Markus28]

What's up with the failed profile check? I don't see where the segmentation fault could come from :(. Apart from that, how does it look?

[Trinkle23897]

not sure what's going wrong. I ran this test locally and it can successfully pass the check without segfault, maybe something went wrong in github container?

[Trinkle23897]

I found the cause: it is because torch==1.10.0

[Trinkle23897]

here

[Markus28]

Awesome! Thanks for the quick review and the polishing 👍

[ultmaster]

I have my own env worker here: https://github.com/microsoft/nni/blob/1eced0a7bff9d4de22487e7cfa45d41ce1f66605/nni/retiarii/strategy/_rl_impl.py#L25

But now it's broken because I need to implement two new methods.

I tried to update my code and delete the __getattr__ method, but the updated version turns out to be imcompatible the tianshou 0.4.4.

How can I implement an env worker that works with both versions of tianshou? Implement both of __getattr__ and get_env_attr? or use if-clause to check tianshou version?

---

My workaround here:

[Trinkle23897]

how about try...except ?

[ultmaster]

Not a good idea. Part of environment is user code. I don't want getattr to be run twice or catch unwanted errors.

[Trinkle23897]

Also a small request: can we have a unit test for your nni integration?

[ultmaster]

Sure. The related test code is here: https://github.com/microsoft/nni/blob/d50b46650bdc9e86aec8b2e0c07204c599310c9d/test/ut/retiarii/test_strategy.py#L144

I'm not sure whether it's handy right now because you need to install NNI from source (instead of from pip) for the tests.

---

Or maybe tianshou can have a built-in support for Multi-thread env worker?

[ultmaster]

I'm experiencing issues as my CI upgrading to tianshou 0.4.6 breaks my env worker again. It looks like another interface change.

>   self.workers = [worker_fn(fn) for fn in env_fns]
E   TypeError: Can't instantiate abstract class MultiThreadEnvWorker with abstract method send

I understand that EnvWorker might not be considered as a public API. But as I'm implementing a 3rd-party env worker, it becomes a burden for me to be always compatible with all kinds of versions. I've already written 3 different version of env worker for tianshou 0.4.4 and 0.4.5 and 0.4.6, and unfortunately they are not compatible with each other.

[Trinkle23897]

Could you please upgrade to the newest version regardless of previous one? I'm super sorry about that happens but actually there's no CI that can secure this thing would never happen again.

[Trinkle23897]

Or maybe tianshou can have a built-in support for Multi-thread env worker?

This is in EnvPool

[ultmaster]

I've sent a patch. It's currently going through our CI.

microsoft/nni#4586

[ultmaster]

Or maybe tianshou can have a built-in support for Multi-thread env worker?

This is in EnvPool

Looks like yet another framework under intense development. :)

[Trinkle23897]

I can make a post version tmr to fix this issue. (Also I just found another issue of consistency and it's critical to be fixed)

[Trinkle23897]

fixed in #536