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

Api changes #18

Merged
merged 228 commits into from
Jun 7, 2022
Merged

Api changes #18

merged 228 commits into from
Jun 7, 2022

Conversation

eddiebergman
Copy link
Contributor

@eddiebergman eddiebergman commented Apr 20, 2022

A major API overhaul to make cleaner separation between different OS's and to make it easier to document and understand all the things going on.

For reviewing, I'd highly recommend just reading the code outside of the github differ as there no diffable changes.

The README.md is a good spot to start to see the changes/features.

@eddiebergman
Copy link
Contributor Author

It appears so and I don't know why, would you be able to try it out? I removed all occurences of time.process_time yet mac still seems to timeout

@mfeurer
Copy link
Contributor

mfeurer commented May 5, 2022

It appears so and I don't know why, would you be able to try it out? I removed all occurences of time.process_time yet mac still seems to timeout

Hey, how about we get going with a version that doesn't yet include OSX and make OSX a second PR? Keeping an overview in this one is becoming hard.

@eddiebergman
Copy link
Contributor Author

I agree, I removed mac from the tests. Windows and Linux should pass and then we can merge and leave for another time.

@eddiebergman
Copy link
Contributor Author

eddiebergman commented May 5, 2022

@renesass @mfeurer Finished, mac testing is disabled for now and a note was added to the README. I think it's ready for merging if you are both okay with it. I know auto-sklearn has the version locked <0.7 but there might be other libraries effected. We should warn them before doing any merge?

I would also propose to make it a 1.0.0 release?

@eddiebergman
Copy link
Contributor Author

Here is a search within our org:
https://github.com/search?q=pynisher+user%3Aautoml&type=Code&ref=advsearch&l=&l=

There are a few libraries new and old that use it, most don't have any upper bound on Pynisher so I would go and raise an issue with each of them that there is major API breaking changes.

@renesass
Copy link
Contributor

renesass commented May 5, 2022

Sounds good. But let me fix the Mac tests. I think it shouldn't be much overhead. Is tomorrow fine?

@eddiebergman
Copy link
Contributor Author

Sounds good. But let me fix the Mac tests. I think it shouldn't be much overhead. Is tomorrow fine?

Sure, You'll have to re-enable them in the pytest.yml once you're done.

@renesass
Copy link
Contributor

renesass commented May 5, 2022

And we definitely should push for 1.0.0. :)

@eddiebergman
Copy link
Contributor Author

The autopytorch team currently have multiple branches and version for benchmarking, I left an issue but I would hold off with merging until they are done as it would break all the experiments.

@mfeurer
Copy link
Contributor

mfeurer commented May 5, 2022

Sounds good. But let me fix the Mac tests. I think it shouldn't be much overhead. Is tomorrow fine?

Sure, You'll have to re-enable them in the pytest.yml once you're done.

Sound good to me, too. I'll wait then with the final review until I get an okay from you.

The autopytorch team currently have multiple branches and version for benchmarking, I left an issue but I would hold off with merging until they are done as it would break all the experiments.

We don't have to immediately release the new version and can give them a bit of time. Also, as they use the pynisher via SMAC, the next SMAC version could introduce an upper bound on the pynisher version to simplify migration.

@eddiebergman
Copy link
Contributor Author

Using this code search filter, there's about 10 libraries in AutoML that need to be informed as they explicitly state a dependancy on it

https://github.com/search?q=pynisher+user%3Aautoml+filename%3Arequirements.txt&type=Code&ref=advsearch&l=&l=

eddiebergman added a commit to automl/nasbench301 that referenced this pull request May 5, 2022
@mfeurer
Copy link
Contributor

mfeurer commented May 6, 2022

I'd say only HPOLib1.5 is a real library, but that's neither used nor maintained any more.

@renesass
Copy link
Contributor

renesass commented May 6, 2022

I'm still not really satisfied that in the MacLimit we raise a RuntimeException now if memory is not supported.

@eddiebergman
Copy link
Contributor Author

I'm still not really satisfied that in the MacLimit we raise a RuntimeException now if memory is not supported.

I'm not sure what we should do to be honest, I good application will check support before trying to pynish anything using support. Otherwise, I think it's better we fail then to silently pretend anything is limited. Open to suggestions though

Btw, So happy to see those mac tests pass, just pre-commit and we're good :)

@eddiebergman
Copy link
Contributor Author

I'm going to make this a 1.0.0 release just to signify the API changes and the support for windows and Mac. I will ask the autopyotrch team today about when is a good time to release, as they would have to update all of their branches they're benchmarking.

@mfeurer
Copy link
Contributor

mfeurer commented May 10, 2022

@renesass would this be ready for a final review?

@renesass
Copy link
Contributor

renesass commented May 10, 2022

I had a quick look and I'm fine with how it is rn :)
I will prepare an update for SMAC accordingly.

@eddiebergman
Copy link
Contributor Author

eddiebergman commented May 13, 2022

Got the go ahead to go break AutoPytorch, we can merge whenever we're ready

@renesass
Copy link
Contributor

SMAC is ready to merge.

@eddiebergman eddiebergman merged commit c43216c into master Jun 7, 2022
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