Skip to content
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

Fix dtype casting issue with numpy in LlamaTune adapter #945

Merged
merged 7 commits into from
Jan 30, 2025

Conversation

kkanellis
Copy link
Collaborator

Pull Request

Description

This PR addresses a breaking change in NumPy's universal function (ufunc) behavior in version 2.0, specifically affecting np.clip in the LlamaTune adapter code.

Key Changes:

  • Resolves an issue where configuration retrieval fails due to type casting differences
  • Explicitly casts np.clip output from NumPy data type (e.g., int64) to native Python type (e.g., int)

Problem Context:
In NumPy 2.0+, when ufuncs receive Python scalars as input, the output is no longer automatically cast to Python scalars. This prevents retrieving previously suggested configurations stored in self._suggested_configs dict during self.inverse_transform method calls.

Technical Solution:
Implement explicit type casting to ensure consistent configuration representation in self._suggested_configs across self.transform and self.inverse_transform method calls.


Type of Change

  • 🛠️ Bug fix

Testing

  • Environment: Ubuntu 22.04 with Python 3.13
  • NumPy Versions Tested: 1.26.4 and 2.2.2
  • Verification: Existing test suite passed across both versions

@kkanellis kkanellis requested a review from a team as a code owner January 25, 2025 00:23
@bpkroth
Copy link
Contributor

bpkroth commented Jan 28, 2025

Thanks for digging into this. Adjusting slightly to also remove the numpy version pinning.

bpkroth
bpkroth previously approved these changes Jan 28, 2025
Copy link
Contributor

@bpkroth bpkroth left a comment

Choose a reason for hiding this comment

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

Made a few small tweaks, but LGTM. Thanks for tracking this down!

@bpkroth bpkroth self-requested a review January 28, 2025 20:05
@bpkroth
Copy link
Contributor

bpkroth commented Jan 28, 2025

Actually, it looks like this is still failing a test:

pytest -n0 mlos_bench/mlos_bench/tests/optimizers/toy_optimization_loop_test.py -k smac

...

    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/opt/conda/envs/mlos/lib/python3.13/site-packages/smac/main/smbo.py", line 431, in save
    self._runhistory.save(path / "runhistory.json")
    ~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/conda/envs/mlos/lib/python3.13/site-packages/smac/runhistory/runhistory.py", line 805, in save
    assert self._running == len(self._running_trials)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError

That doesn't happen with numpy<2.0, but with numpy 2.2.2 it does happen whether I use the version check or the try/except.

@kkanellis can you please take another look?

Thanks!

@bpkroth
Copy link
Contributor

bpkroth commented Jan 29, 2025

@kkanellis were you able to track the source of this down at all?
From my quick glance, you're right that smac3 updated to v2.3.0 and started allowing numpy 2.0 (prior apparently both we and smac3 were disallowing it).
However, now there's a different bug with smac3.
Would be good to at least track that down and submit an Issue upstream to them.

@kkanellis
Copy link
Collaborator Author

I will try to track this down tomorrow.

In any case, since this look like a different bug/issue with SMAC, I propose merging this (w/o the change on the numpy version), and open a new issue just for this.

Copy link
Contributor

@bpkroth bpkroth left a comment

Choose a reason for hiding this comment

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

As discussed, merging as is since we are fairly confident this fixes one issue.
Will lift the numpy 2.0 restriction later once the SMAC issue is tracked down.

@bpkroth bpkroth enabled auto-merge (squash) January 30, 2025 03:43
@bpkroth bpkroth merged commit e94989e into microsoft:main Jan 30, 2025
16 checks passed
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.

LlamaTune incompatible with numpy>=2.0
2 participants