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

Enable context for SMAC Optimizer #741

Closed
wants to merge 52 commits into from

Conversation

jsfreischuetz
Copy link
Contributor

No description provided.

.vscode/launch.json Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
@@ -108,7 +113,7 @@ def __repr__(self) -> str:
)
return f"{self.name}({opt_targets},config={self._config})"

def __enter__(self) -> 'Optimizer':
def __enter__(self) -> "Optimizer":
Copy link
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?

@jsfreischuetz
Copy link
Contributor Author

@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
Contributor

bpkroth commented May 17, 2024

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

@@ -68,25 +68,30 @@ def __init__(self,
self._seed = int(config.get("seed", 42))
self._in_context = False

experiment_id = self._global_config.get('experiment_id')
experiment_id = self._global_config.get("experiment_id")
Copy link
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
Contributor 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

Copy link
Contributor

@bpkroth bpkroth May 22, 2024

Choose a reason for hiding this comment

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

Yeah, @motus @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
Contributor 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