Skip to content

[tune] implement shim instantiation#10456

Merged
richardliaw merged 47 commits intoray-project:masterfrom
sumanthratna:shim-instantiation
Sep 5, 2020
Merged

[tune] implement shim instantiation#10456
richardliaw merged 47 commits intoray-project:masterfrom
sumanthratna:shim-instantiation

Conversation

@sumanthratna
Copy link
Member

@sumanthratna sumanthratna commented Aug 31, 2020

Why are these changes needed?

makes creating search algorithms based on a string easier (useful for CLIs where users define search alg and scheduler to use)

Related issue number

related: #9969, #10401, #10444, #10451 (closes #10451)

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/latest/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested (please justify below)

CC @richardliaw

@sumanthratna sumanthratna changed the title [WIP] Create ray.tune.suggest.create.create_scheduler [WIP] implement shim instantiation Aug 31, 2020
@sumanthratna sumanthratna changed the title [WIP] implement shim instantiation [WIP] [tune] implement shim instantiation Sep 3, 2020
@sumanthratna
Copy link
Member Author

sumanthratna commented Sep 3, 2020

yikes this PR's commits have become way more bloated than they need to be; @richardliaw should I rebase and force-push to reduce the number of commits?

@richardliaw
Copy link
Contributor

Hmm I don't really mind - up to you!

Copy link
Contributor

@richardliaw richardliaw left a comment

Choose a reason for hiding this comment

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

Looks great! Do you mind adding this into the docs too? ray/docs/source/tune/api_docs/{suggestion / schedulers}.rst

@richardliaw
Copy link
Contributor

bump - looks like tests are failing. I think you should do type(scheduler) == type(...) in the unit tests

@sumanthratna
Copy link
Member Author

Looks great! Do you mind adding this into the docs too? ray/docs/source/tune/api_docs/{suggestion / schedulers}.rst

sure, I'll try to do that soon

looks like tests are failing. I think you should do type(scheduler) == type(...) in the unit tests

Oh—I had (lazily) assumed that schedulers overrode the equality operator. Is this something we should implement? I don't think it would be too hard (untested):

def __eq__(self, other):
    if type(self) != type(other):
        return False
    return self.__dict__ == other.__dict__

this is probably better suited for a different PR, though

@richardliaw
Copy link
Contributor

Hm I guess I don't know if those semantics really are needed since you're almost certainly only using 1 scheduler.

@richardliaw
Copy link
Contributor

Thanks for putting together this PR though! Definitely looking forward to getting this merged :)

sumanthratna and others added 2 commits September 4, 2020 08:45
@sumanthratna
Copy link
Member Author

@richardliaw can you take a look at 6dcf1f0 and let me know if I should change anything else? once you take a look I'll add to the schedulers doc too

@richardliaw
Copy link
Contributor

richardliaw commented Sep 4, 2020 via email

@richardliaw
Copy link
Contributor

Hey @sumanthratna looks like the create search alg test is still broken

Don't forget to run ci/format.sh too to address lint test!

@richardliaw richardliaw added this to the Tune/SGD 1.0 Hotlist milestone Sep 4, 2020
@sumanthratna
Copy link
Member Author

Hey @sumanthratna looks like the create search alg test is still broken

@richardliaw ugh I had copied the example from the docs because I wanted to write the test correctly but I was looking at the 0.8.7 docs instead of master...fix is (hopefully) on the way

@richardliaw richardliaw added the P1 Issue that should be fixed within a few weeks label Sep 5, 2020
@richardliaw
Copy link
Contributor

richardliaw commented Sep 5, 2020 via email

@sumanthratna
Copy link
Member Author

we can wait until all tests finish but @richardliaw I think this is finally ready for merging

@richardliaw richardliaw merged commit 54215ff into ray-project:master Sep 5, 2020
@sumanthratna sumanthratna deleted the shim-instantiation branch September 5, 2020 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 Issue that should be fixed within a few weeks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[tune] shim instantiation of search algorithms

2 participants