You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/05/06 18:13:50 UTC
[GitHub] [tvm] anwang2009 opened a new pull request #7992: Remove minimum seed constraint on XGB Tuner
anwang2009 opened a new pull request #7992:
URL: https://github.com/apache/tvm/pull/7992
Here we remove 500 minimum sample constraint for seeding the XGB Tuner. This constraint caused the XGBoost tuner to silently disregard data from when `load_history` is invoked.
@merrymercy @tkonolige
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on pull request #7992: Remove minimum seed constraint on XGB Tuner
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #7992:
URL: https://github.com/apache/tvm/pull/7992#issuecomment-836289665
According to the discussion, I think we all agree that the threshold of not training a cost model should be configurable instead of a fixed value (whatever it's 500 or 0). @anwang2009 could you update the PR accordingly, and maybe use 500 as the default value for back compatible? If you aim to fully support resume tuning in AutoTVM, it would be better to have a follow up PR to correctly restore the tuning status.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] anwang2009 commented on pull request #7992: Remove minimum seed constraint on XGB Tuner
Posted by GitBox <gi...@apache.org>.
anwang2009 commented on pull request #7992:
URL: https://github.com/apache/tvm/pull/7992#issuecomment-833824796
@comaniac Maybe I'm misunderstanding the XBG tuner behavior -- I assumed that providing 10 records to seed the model is approximately functionally equivalent to the tuner exploring those 10 records on its own from scratch.
My use case is that if a tuning job is interrupted, I want to resume the progress midway. I assumed I could resume progress by invoking `load_history` with the records I'd seen so far. If seeding the records won't allow me to do this efficiently, do you know of another way?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jroesch commented on pull request #7992: Remove minimum seed constraint on XGB Tuner
Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #7992:
URL: https://github.com/apache/tvm/pull/7992#issuecomment-833830299
@comaniac even if we want a 500 default this should not be hardwired into the code, usually magic numbers are considered a bad coding practice, we should probably at least name this constant make this behavior configurable since 500 is arbitrary hyper parameter of the system
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] merrymercy edited a comment on pull request #7992: Remove minimum seed constraint on XGB Tuner
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #7992:
URL: https://github.com/apache/tvm/pull/7992#issuecomment-834784550
I agree with @comaniac. We can expost this magic number to the top-level api and set the default value as 500.
You can then ues any values you want.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] merrymercy commented on pull request #7992: Remove minimum seed constraint on XGB Tuner
Posted by GitBox <gi...@apache.org>.
merrymercy commented on pull request #7992:
URL: https://github.com/apache/tvm/pull/7992#issuecomment-834784550
I agree with @comaniac. We can expost this magic number to the api and set the default value as 500.
You can then ues any values you want.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jroesch commented on pull request #7992: Remove minimum seed constraint on XGB Tuner
Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #7992:
URL: https://github.com/apache/tvm/pull/7992#issuecomment-838895182
@comaniac @merrymercy thanks for reviewing!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] anwang2009 commented on pull request #7992: Remove minimum seed constraint on XGB Tuner
Posted by GitBox <gi...@apache.org>.
anwang2009 commented on pull request #7992:
URL: https://github.com/apache/tvm/pull/7992#issuecomment-837023732
thanks for the comments. @comaniac I added a `min_seed_records=500` parameter to `load_history`, PTAL
I also updated the `update` function in `model_based_tuner.py` to call `self.visited.add` so that I can use the `update` function to resume progress. I explain my thinking further in an added comment in the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] anwang2009 commented on pull request #7992: Remove minimum seed constraint on XGB Tuner
Posted by GitBox <gi...@apache.org>.
anwang2009 commented on pull request #7992:
URL: https://github.com/apache/tvm/pull/7992#issuecomment-837377574
@comaniac Added tests, do they look sufficient?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jroesch merged pull request #7992: Remove minimum seed constraint on XGB Tuner
Posted by GitBox <gi...@apache.org>.
jroesch merged pull request #7992:
URL: https://github.com/apache/tvm/pull/7992
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] merrymercy edited a comment on pull request #7992: Remove minimum seed constraint on XGB Tuner
Posted by GitBox <gi...@apache.org>.
merrymercy edited a comment on pull request #7992:
URL: https://github.com/apache/tvm/pull/7992#issuecomment-834784550
I agree with @comaniac. We can expost this magic number to the top-level api and set the default value as 500.
You can then ues any values you want.
But as @comaniac said, just using this function cannot achieve your specific goal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on pull request #7992: Remove minimum seed constraint on XGB Tuner
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #7992:
URL: https://github.com/apache/tvm/pull/7992#issuecomment-833920681
> @comaniac Maybe I'm misunderstanding the XBG tuner behavior -- I assumed that providing 10 records to seed the model is approximately functionally equivalent to the tuner exploring those 10 records on its own from scratch.
>
> My use case is that if a tuning job is interrupted, I want to resume the progress midway. I assumed I could resume progress by invoking `load_history` with the records I'd seen so far. If seeding the records won't allow me to do this efficiently, do you know of another way?
AFAIK by looking at the implementation, starting from the second iteration XGBTuner uses `self.xs` and `self.ys` to train the cost model. However, we didn't add loaded records to `self.xs` and `self.ys`, so they won't perform as you expected. If the goal is to support resume tuning, we need to add history records to `self.xs`,`self.ys`, and `self.visited` in `load_history`.
> @comaniac even if we want a 500 default this should not be hardwired into the code, usually magic numbers are considered a bad coding practice, we should probably at least name this constant make this behavior configurable since 500 is arbitrary hyper parameter of the system
I agree that 500 is ad-hoc and should be configurable, so I'm fine with exposing this number to the user API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on pull request #7992: Remove minimum seed constraint on XGB Tuner
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #7992:
URL: https://github.com/apache/tvm/pull/7992#issuecomment-837038252
The change LGTM, thanks. Could you add a test case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org