Skip to content

Enable context for SMAC Optimizer - #741

Closed
Johannes Freischuetz (jsfreischuetz) wants to merge 52 commits into
microsoft:mainfrom
jsfreischuetz:main
Closed

Johannes Freischuetz (jsfreischuetz) wants to merge 52 commits into
microsoft:mainfrom
jsfreischuetz:main

Conversation

@jsfreischuetz

Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread .vscode/launch.json
Comment thread .vscode/settings.json
Comment thread conda-envs/condaenv.ib_tklms.requirements.txt Outdated
return f"{self.name}({opt_targets},config={self._config})"

def __enter__(self) -> 'Optimizer':
def __enter__(self) -> "Optimizer":

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.

There's a whole bunch of style-only changes in here that makes reviewing a little difficult.

Can you please revert those, or separate them out to a different PR?

Comment thread mlos_core/notebooks/BayesianOptimization.ipynb
@jsfreischuetz

Copy link
Copy Markdown
Collaborator Author

Johannes Freischuetz (@jsfreischuetz) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@ agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@ agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@ agree company="Microsoft"

Contributor License Agreement

@ agree

@bpkroth

Copy link
Copy Markdown
Contributor

I think we agreed to stall this on until #744 is done to make these changes easier to consume.

self._in_context = False

experiment_id = self._global_config.get('experiment_id')
experiment_id = self._global_config.get("experiment_id")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

here and everywhere: let's separate cosmetic fixes from functional changes. mixing the two bloats up the diff and makes reviewing PRs much harder. Please keep the essential functionality in this PR and make the diff as small as possible and move all code annotations, style, and docstring improvements to a separate PR. having more PRs is good for your github karma! 😄

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good! #744 should do just the style changes

@bpkroth Brian Kroth (bpkroth) May 22, 2024 •

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.

Yeah, Sergiy Matusevych (@motus) Johannes Freischuetz (@jsfreischuetz) and I already talked about that. #744 is the start of that, but I think will also need some additional work. #746 unearthed some related topics around pyproject.toml settings vs. setup.cfg for using black and how setup and build type dependencies are specified (right now we require conda for certain ones instead of pip, which as we saw, has some issues).

Am currently working on improvements for all of those and will send a split out series of PRs soon for:

  1. pyproject.toml settings and build related improvements
    • annoyed at this one since there's no "include" feature, so it means duplicating some content across files it looks like
  2. adding black and isort Makefile rules, vscode settings, configs, etc.
    • that's a part of this one, but I'm going to restrict it to a smaller set so it's easier to see the changes
  3. requiring them in the build (including all of the reformatting of all files)
    • this will be a larger change, so I want make sure it's just reformatting. Can take that up here or just create a new PR
  4. adding the revision from 3 in the .gitrevisions list to ignore it from most git blame types of analysis

@jsfreischuetz

Copy link
Copy Markdown
Collaborator Author

Replaced with #751

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