Skip to content

[FEAT] Option to customize the hyperparameter tuning behavior of Ray #1286

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

JQGoh
Copy link
Contributor

@JQGoh JQGoh commented Mar 11, 2025

Main objective is to provide a way to customize the hyperparameter tuning behavior of Ray. See issues

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@JQGoh JQGoh force-pushed the feat/ray-customized branch 2 times, most recently from aebcea1 to 644a38c Compare March 13, 2025 00:36
# but the search algorithm was already instantiated with a search space.
# Make sure that `config` does not contain any more parameter definitions
# - include them in the search algorithm's search space if necessary.
model_copy = deepcopy(self)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elephaint would love to have your initial round of review on this. This works but I would like to investigate further and hear more from others.

Without this, it raises the mentioned error in the previous failed job like https://github.com/Nixtla/neuralforecast/actions/runs/13796357322/job/38588846450?pr=1286

@marcopeix
Copy link
Contributor

Hi @JQGoh thanks a lot for the PR! I have few comments and questions.

  • First of all, I think it's great that you're tackling this and we're definitely on the right track.
  • Running the tutorial notebook on my end, I see that the predictions are exactly the same, whether we use the BHOB scheduler or not. Is that expected? It might be that AirPassengers is too simple, but I wanna make sure you see the same (in which case, we have to modify the conclusion of the tutorial)
  • Can we include a time budget? I see you tagged that issue, but I don't see it in the tutorial.
  • Can you merge main into the branch please?

Thanks a lot for your work!

@JQGoh
Copy link
Contributor Author

JQGoh commented Apr 10, 2025

Hi @JQGoh thanks a lot for the PR! I have few comments and questions.

  • First of all, I think it's great that you're tackling this and we're definitely on the right track.
  • Running the tutorial notebook on my end, I see that the predictions are exactly the same, whether we use the BHOB scheduler or not. Is that expected? It might be that AirPassengers is too simple, but I wanna make sure you see the same (in which case, we have to modify the conclusion of the tutorial)
  • Can we include a time budget? I see you tagged that issue, but I don't see it in the tutorial.
  • Can you merge main into the branch please?

Thanks a lot for your work!
@marcopeix

  • I just realized that the tutorial example is not updated to reflect the intended behavior (it has an error and misses the chart). I think I will have more time to revisit this closer to the end of the month. But I agree with you that for a simple optimization task, it tends to lead to the same optimized setting during my test.

  • including a time budget in the tutorial sounds good to me. The design is supposed to be flexible such that we don't keep introducing arguments that control the optimization behavior.

  • I can help to merge main into the branch first

  • My main concern is my "hack" of introducing the deepcopy of the model itself, see this
    [FEAT] Option to customize the hyperparameter tuning behavior of Ray #1286 (comment)

This happens when we do have conformal predictions intervals setting and we execute fit() multiple times. I have not got time to investigate and fully understood behavior, though the hack can bypass the PR failure.

@marcopeix
Copy link
Contributor

@JQGoh I tried some approaches on my end to avoid using deepcopy, but nothing worked. I don't think there's a simple solution to it. For now, I would keep it as is, and make sure that the functionality works, and the tutorial is adjusted. We can always think of a better approach later on. I believe the advantages outweigh the drawbacks here.

@JQGoh
Copy link
Contributor Author

JQGoh commented Apr 15, 2025

@JQGoh I tried some approaches on my end to avoid using deepcopy, but nothing worked. I don't think there's a simple solution to it. For now, I would keep it as is, and make sure that the functionality works, and the tutorial is adjusted. We can always think of a better approach later on. I believe the advantages outweigh the drawbacks here.

@marcopeix Is there any plan to do a release soon? I try to wrap this up by end of the week if possible

@marcopeix
Copy link
Contributor

@JQGoh I tried some approaches on my end to avoid using deepcopy, but nothing worked. I don't think there's a simple solution to it. For now, I would keep it as is, and make sure that the functionality works, and the tutorial is adjusted. We can always think of a better approach later on. I believe the advantages outweigh the drawbacks here.

@marcopeix Is there any plan to do a release soon? I try to wrap this up by end of the week if possible

I want to merge a couple of PRs before making a release (this one included), so no rush!

" # but the search algorithm was already instantiated with a search space. \n",
" # Make sure that `config` does not contain any more parameter definitions \n",
" # - include them in the search algorithm's search space if necessary.\n",
" model_copy = deepcopy(self)\n",
Copy link
Contributor

@elephaint elephaint Apr 16, 2025

Choose a reason for hiding this comment

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

@JQGoh for determining the conformity scores, isn't it sufficient to temporarily change some parameters of the model so that it doesn't give the error?

I can imagine this creates unnecessary (V)RAM load.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elephaint most recent weeks I have been quite well occupied that I have not got more time to have a closer look at this. That was the reason I first tagged you on the deepcopy practice, to have your team's initial review first. When I have more time, I would love to investigate further

@JQGoh JQGoh force-pushed the feat/ray-customized branch from 644a38c to 25f2543 Compare July 10, 2025 20:41
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.

3 participants