Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rllib] cast fcnet_hiddens to list #14308

Merged
merged 1 commit into from
Feb 25, 2021
Merged

Conversation

krfricke
Copy link
Contributor

Why are these changes needed?

Cast config parameter to list to make sure that list operations work

Related issue number

Closes #14304

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@@ -24,7 +24,7 @@ def __init__(self, obs_space: gym.spaces.Space,
model_config, name)
nn.Module.__init__(self)

hiddens = model_config.get("fcnet_hiddens", []) + \
hiddens = list(model_config.get("fcnet_hiddens", [])) + \
model_config.get("post_fcnet_hiddens", [])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
model_config.get("post_fcnet_hiddens", [])
list(model_config.get("post_fcnet_hiddens", []))

, since post_fcnet_hiddens will be tuple if we use HyperOpt to search its best value (similar to fcnet_hiddens). (detail in #14304)

Copy link
Contributor

@sven1977 sven1977 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix @RaphaelCS and @krfricke .

@sven1977 sven1977 merged commit d9e5d5f into ray-project:master Feb 25, 2021
@krfricke krfricke deleted the issue_14304 branch February 25, 2021 09:29
@ghost
Copy link

ghost commented Feb 25, 2021

@sven1977 It seems that my suggestion was not added to the code 😢
Is there anything wrong with my suggestion?

krfricke pushed a commit to krfricke/ray that referenced this pull request Feb 25, 2021
@krfricke
Copy link
Contributor Author

Probably just an oversight when merging the issue - it's good actually. I created a PR to fix this issue in #14354 - thanks again!

@ghost
Copy link

ghost commented Feb 26, 2021

Ah, thanks a lot!

krfricke added a commit that referenced this pull request Mar 22, 2021
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.

[rllib] [tune] Cannot create fcnet when using HyperOptSearch
2 participants