Fix 'list index out of range' Error with 'mlflow_logging': True and 'max_iter': 1#1417
Fix 'list index out of range' Error with 'mlflow_logging': True and 'max_iter': 1#1417Stickic-cyber wants to merge 3 commits into
Conversation
|
Thank you @Stickic-cyber for the PR. Could you please provide more details for the PR? |
|
Hi @thinkall I have updated the pull request and resubmitted it with the following changes: 1.Handling empty manual_run_ids: 2.Safe access to _config_history: |
thinkall
left a comment
There was a problem hiding this comment.
I think a better way is to do search for max_iter = 1. See my comments below.
| if self.manual_log: | ||
| best_mlflow_run_id = self.manual_run_ids[automl._best_iteration] | ||
| if len(self.manual_run_ids) == 0: | ||
| best_mlflow_run_id = self.parent_run_id or mlflow.active_run().info.run_id |
There was a problem hiding this comment.
In current design, max_iter = 1 makes no sense to FLAML. There will be no mlflow run created for it. When max_iter>1, self.mlflow_integration.record_state(self, search_state, estimator) will be called to create mlflow runs to record states.
The key here is that no run is created and appended to manual_run_ids. Set it to parent_run or current active run is not correct.
I see two ways to fix the issue:
-
Don't skip search when max_iter = 1, it's acceptable as one trial won't take a lot of time, thus won't affect the test process (I think people set max_iter to 1 for quick test purpose).
-
Add mlflow_integration.record_state to the logic of max_iter < 2.
Need to test which one is better, i.e., won't introduce new bugs.
There was a problem hiding this comment.
@Stickic-cyber it seems that you missed my detailed comments on your code changes.
| if "ml" in conf.keys(): | ||
| conf = conf["ml"] | ||
| conf = {} | ||
| if automl._best_iteration in automl._config_history:### |
There was a problem hiding this comment.
If we do search for max_iter=1, this change won't be needed.
thinkall
left a comment
There was a problem hiding this comment.
@Stickic-cyber , please follow https://microsoft.github.io/FLAML/docs/Contribute#pre-commit to fix format issue.
|
Thanks for your review and suggestions! I see your point regarding 1.Running the search even when 2.Adding mlflow_integration.record_state when I'll check which one works best without introducing new issues. Also, I'll follow the pre-commit guidelines to fix the formatting. Thanks again for the feedback! I'll update the PR accordingly. |
|
After testing, I believe Option 1 is more suitable (my previous attempt was somewhat redundant). If we were to use Option 2, adding logic to the |
@microsoft-github-policy-service agree |
I don't understand this part. |
I actually think we should go with this solution as it seems to be the safest option. |

Why are these changes needed?
This PR handles empty manual_run_ids to prevent 'list index out of range' error.
The fix ensures that if manual_run_ids is empty, best_mlflow_run_id falls back to self.parent_run_id or the active MLflow run ID before attempting to access manual_run_ids.
Related issue number
closes #1416
Checks