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

Lazy loading torch - part 1 #119

Merged
merged 3 commits into from
Feb 29, 2024
Merged

Lazy loading torch - part 1 #119

merged 3 commits into from
Feb 29, 2024

Conversation

rjavadi
Copy link
Contributor

@rjavadi rjavadi commented Feb 7, 2024

This is the first part of optimizing torch imports.

All tests passed on my machine with python 3.10 and 3.11.

@rjavadi rjavadi force-pushed the torch-import-separation branch from bd9ae99 to 4a29f1b Compare February 7, 2024 20:11
@rjavadi rjavadi marked this pull request as ready for review February 7, 2024 20:29
@Scienfitz Scienfitz added the enhancement Expand / change existing functionality label Feb 8, 2024
@AdrianSosic AdrianSosic force-pushed the torch-import-separation branch from cabfb8e to 7b4bfad Compare February 12, 2024 08:06
Copy link
Collaborator

@AdrianSosic AdrianSosic left a comment

Choose a reason for hiding this comment

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

Hi @rjavadi, thanks for the PR and please apologize that no one of us responded last week. Martin was OOO + Alex and I were fighting to get the new 0.7.3 released.

I've added some in-line comments below. Apart from that, here some general feedback:

  • Your branch was again ab bit messe (~40 commits) so I rebased it and created a backup of the original version. Since your changes are pretty much orthogonal to what is currently happening in the repo, it would be really nice if you could try to rebase instead of merge (see also our new contributing guideline). Let me know if you have questions 🙃
  • Also, have a look at the general style the of the existing commits and changelog messages
  • Finally, to better organize our branches, it would be nice if you could place your next branch into an appropriate "folder". For the current PR, you could have used e.g. "refactor/torch_imports" or similar 👍🏼

CHANGELOG.md Outdated Show resolved Hide resolved
baybe/scaler.py Outdated Show resolved Hide resolved
baybe/scaler.py Outdated Show resolved Hide resolved
baybe/utils/dataframe.py Outdated Show resolved Hide resolved
baybe/utils/interval.py Outdated Show resolved Hide resolved
baybe/utils/interval.py Show resolved Hide resolved
Copy link
Collaborator

@AVHopp AVHopp left a comment

Choose a reason for hiding this comment

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

Nothing more to add, so I'll already approve :)

baybe/scaler.py Outdated Show resolved Hide resolved
baybe/scaler.py Outdated Show resolved Hide resolved
baybe/searchspace/continuous.py Outdated Show resolved Hide resolved
baybe/searchspace/continuous.py Outdated Show resolved Hide resolved
baybe/utils/interval.py Show resolved Hide resolved
@Scienfitz
Copy link
Collaborator

@rjavadi what is the status of this PR? I must admit I cant really follow, could you add comments in threads if youve addressed them and/or have questions/suggestions?

Also, what has happened to the commit history?

@rjavadi
Copy link
Contributor Author

rjavadi commented Feb 22, 2024

@rjavadi what is the status of this PR? I must admit I cant really follow, could you add comments in threads if youve addressed them and/or have questions/suggestions?

Also, what has happened to the commit history?

It's ready to merge. I marked the conversations as resolved.

I fixed the history.

@rjavadi rjavadi force-pushed the torch-import-separation branch 3 times, most recently from 00485c2 to c4f1920 Compare February 27, 2024 05:41
@AVHopp
Copy link
Collaborator

AVHopp commented Feb 27, 2024

@rjavadi first of all, thanks for your contribution :) I've seen that the tests currently fail, could maybe investigate the following?
It seems like there is one reference to Tensor that cannot be resolved when building the doc, the following seems to be the issue:
image
The same issue seems to happen when the other tests are being executed.

@rjavadi rjavadi requested a review from AdrianSosic February 27, 2024 18:01
@rjavadi rjavadi dismissed AdrianSosic’s stale review February 27, 2024 18:03

I fixed branch history and rebased latest updates on main branch. I'm dismissing this review because it appears as a "change request" and prevents merging.

@rjavadi rjavadi requested a review from Scienfitz February 27, 2024 18:03
@Scienfitz Scienfitz force-pushed the torch-import-separation branch from 1295ae3 to 2266102 Compare February 29, 2024 15:53
Copy link
Collaborator

@Scienfitz Scienfitz left a comment

Choose a reason for hiding this comment

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

thanks @rjavadi :)

@Scienfitz Scienfitz merged commit 92032f0 into main Feb 29, 2024
10 checks passed
@Scienfitz Scienfitz deleted the torch-import-separation branch February 29, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Expand / change existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants