Skip to content

fix(automl): enable limit_resource on LRL1Classifier (#1612) - #1634

Open
Abhinav Maurya (Fire162) wants to merge 6 commits into
microsoft:mainfrom
Fire162:fix/lrl1-resource-limit
Open

Abhinav Maurya (Fire162) wants to merge 6 commits into
microsoft:mainfrom
Fire162:fix/lrl1-resource-limit

Conversation

@Fire162

Copy link
Copy Markdown

Why are these changes needed?

In flaml/automl/model.py, SKLearnEstimator.fit() checks getattr(self, "limit_resource", None) before wrapping execution in the limit_resource context manager. While LRL2Classifier declares limit_resource = True, LRL1Classifier omitted this attribute.

Because lrl1 is included in the default classification estimator list, training LRL1Classifier with a specified time_budget bypassed resource and timeout limiting entirely, causing long-running fits to overrun budgets significantly on wide or many-class datasets.

This change:

  1. Adds limit_resource = True to LRL1Classifier in flaml/automl/model.py, aligning it with LRL2Classifier.
  2. Adds unit tests in test/automl/test_model.py verifying that LRL1Classifier.limit_resource is True, fit respects budget constraints, and limit_resource is invoked during execution.

Related issue number

Closes #1612

Checks

LRL1Classifier omitted limit_resource = True, causing budgeted training
in AutoML.fit to bypass resource and timeout limits. This adds
limit_resource = True matching LRL2Classifier and unit tests verifying
that resource limits are applied during fit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new invocation test fails on Windows and needs to skip platforms without resource limiting.

1 open issue
What changed in this PR

Enables existing resource limiting for L1 logistic regression, addressing budget overruns reported in #1612.

Changes:

  • Sets LRL1Classifier.limit_resource = True, matching L2.
  • Adds tests for the flag, budgeted fitting, and limiter invocation.
File Description
test/​automl/​test_model.py Adds L1 resource-limiting regression tests.
flaml/​automl/​model.py Enables resource limiting for LRL1Classifier.

Review effort: Balanced

Comment thread test/automl/test_model.py


def test_lrl1_resource_limit_called(monkeypatch):
import flaml.automl.model as model_mod

@thinkall Li Jiang (thinkall) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall review of the complete PR: changes requested for a deterministic Windows test failure.

The LRL1 resource-limit flag matches LRL2 and reuses the existing production platform guard. The added invocation assertion does not respect that guard. Running the two new tests on Windows produced 1 passed and 1 failed: test_lrl1_resource_limit_called asserts a call even though resource is unavailable and BaseEstimator.fit intentionally bypasses the limiter. Please make that test capability-aware; do not remove the production guard. The review-code harness and current Balanced Copilot review agree on this blocker.

Posted by thinkall-agent-auto-reviewer

Comment thread test/automl/test_model.py
X, y = make_classification(50, 5, random_state=0)
lr = LRL1Classifier()
lr.fit(X, y, budget=1.0)
assert len(called) > 0, "limit_resource was not invoked for LRL1Classifier"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This assertion fails on supported Windows platforms. model_mod.resource is None there, so BaseEstimator.fit deliberately does not enter limit_resource, regardless of the new LRL1 flag. I reproduced the failure with python -m pytest test/automl/test_model.py::test_lrl1 test/automl/test_model.py::test_lrl1_resource_limit_called -q (1 passed, 1 failed). Skip the invocation-specific case when resource is unavailable, or assert the appropriate call/no-call behavior for each capability while retaining the cross-platform fit smoke test.

Posted by thinkall-agent-auto-reviewer

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The invocation test needs to account for Windows, where resource limiting is intentionally unavailable.

1 open issue

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The limiter-invocation test needs to account for Windows, where resource limiting is unavailable.

1 open issue

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The limiter-invocation test needs to account for Windows, where resource limiting is unavailable.

1 open issue

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The invocation test needs a platform guard because resource limiting is bypassed on Windows, causing its assertion to fail.

1 open issue

Review effort: Balanced

@thinkall Li Jiang (thinkall) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed commits 4d9553cefbb14233782ccfd928264bfde5ca739c -> 6d5a3a774153cf7836d651b90b73ad48e33694bb. The only delta is main's website/yarn.lock update of follow-redirects 1.16.0 -> 1.16.1. One explicitly head-bound review-code Low execution (actual runtime gpt-6.1-sol) found no new issue in this identical inherited patch. Controller Git comparisons verify all non-lockfile tree entries are unchanged. This is a scoped merge-delta review, not a fresh clean signoff for unchanged feature code.

Version/URL/caret/SRI shape checks pass. No independent local registry/tarball verification, package smoke test or website rebuild is claimed. Previous feature findings and runtime validation limits remain preserved.

Changes remain required. The existing Windows test blocker remains: test_lrl1_resource_limit_called still requires a limiter call when resource is unavailable and production deliberately bypasses that limiter. The test and production guard are unchanged by this merge. The prior Windows result was one passed and one failed; this cycle does not claim to rerun that native test. The original changes-requested review remains applicable.

At the 2026-10-26T12:13:44.943946+00:00 gate snapshot, actual current-head workflows were: Build in_progress/no conclusion; Code formatting completed/success; docs completed/success. Copilot has Balanced review without an Approval recommended verdict; 2 review thread(s) remain unresolved. Gate approval is not successful CI completion, and no new PR approval is being submitted while required gates remain unmet.

Posted by thinkall-agent-auto-reviewer

This branch has not been deployed

No deployments
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.

lrl1 can overrun time_budget by orders of magnitude (770x on Dionis)

3 participants