-
Notifications
You must be signed in to change notification settings - Fork 2
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
🏗️ Integrate lnschema-core
into lamindb
#921
Merged
Merged
Changes from all commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
1560475
🚚 Prepare migration away from lnschema-core
falexwolf 25c4f71
🚚 Rename lnschema_core to lamindb
falexwolf ee11da1
💚 Fix
falexwolf acb66ae
♻️ Install bionty from correct branch
falexwolf 522d6ed
💚 Fix installation
falexwolf 63a932f
💚 Fixes
falexwolf 45a33cc
💚 More fixes
falexwolf f591899
💚 Fix
falexwolf b75ac9b
💚 Start to migrate existing instances
falexwolf d83b451
💚 Fix
falexwolf da81321
💚 Fix
falexwolf 3b03292
💚 Fix
falexwolf c0b8bc6
💚 Replace laminlabs/lamindata with laminlabs/lamin-site-assets
falexwolf 8108c6d
♻️ Add some logging
falexwolf b5fd14d
♻️ Organize migration process
falexwolf e552f44
💚 Fix
falexwolf e1a1db2
♻️ Leave core within schema serialization because the hub hardcodes s…
falexwolf b4a71cb
💚 Fix for other schema modules
falexwolf c08052d
💚 Fixes
falexwolf f030859
💚 Fix
falexwolf a8a55b4
♻️ More consistency
falexwolf 8975b5f
💚 More fixes
falexwolf c3f9f38
💚 Try fixing lamindb install
falexwolf 79c0c3d
💚 Fix noxfile
falexwolf 8ea566b
💚 Remove print statement
falexwolf 1738ba4
💚 Fix install
falexwolf 15d3791
🔥 Remove lnschema-core as a dependency
falexwolf 8761094
💚 Fix
falexwolf 238bb46
♻️ Refactor lamindb
falexwolf 9b65980
💚 Fix import
falexwolf 895b4b2
🔊 More logging
falexwolf a96cc7c
💚 Fix
falexwolf File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid this is much slower, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it be much slower?
(1) Do you mean because of a few additional dependencies that lamindb has over lamindb_setup? If that's the reason, I'm pretty sure it should be negligible. If it's not negligible we ought to think through optimizing import time on lamindb.
(2) If something else: 🤔 -- I can only speculate that you're having the auto-connect behavior in mind. Also here there is no additional slow-ness because you need to have django setup (connection established, if you will) also in the case of lnschema_core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean 1), yes. lamindb is much much heavier in terms of imports, and I don’t really think it can be improved. We definitely need to test the import times. In my opinion significant degradation of performance is not justified by more convenient code organization (and I am not even sure about convenience here). lamin-cli is already pretty slow and we shouldn’t make it even slower I think.
In summary I think we need to check the performance carefully before going forward with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though I don't believe it, let's assume that lamindb is so much heavier that it leads to noteworthy degradation of import times; but even if noteworthy, it's hard to believe that they'd ever degrade more than 300ms.
Now consider that
from lamindb.models import Storage
and all similar statements is a dynamic import inlamindb-setup
. It has to be because django needs to be setup before it runs through.All instances in which the dynamic import is being run are cases in which run time is a few seconds. The worst case scenario of 300ms then isn't nice, but is also no big deal.
I've played with this for the past hours and I don't notice any slow down. If anything, I notice that several parts of the UX are faster (because I disentangled some recursive imports).
This is not at all about convenience. It's about:
Points 2. and 3. are enabled through 1. -- I imagine that 1. will enable more fundamental code organization improvements like 2. and 3. beyond easing operational complications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not that optimistic about performance. I know that it is fine on Linux (and probably macOS), but on windows it is already inconveniently slow and probably will be even slower with these changes. On windows it looks like additional imports make a difference.
Re part 2, I don’t entirely get why 1 and 2 are really needed, but we can discuss of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this will make a positive difference on windows also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also systems with pretty slow import of modules, like Helmholtz compute cluster, we need to have them in mind also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I wasn't aware of this at all. What is inconveniently slow? Can you make an issue on the
lamindb
repo and we discuss there?I hope so!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I am pretty sure we discussed this once or twice but never did anything. I will make an issue with measured loading times vs Linux. Also maybe on Helmholtz cluster (I believe they have pretty standard setup for compute clusters).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw import times on Helmholtz cluster are pretty abysmal at times.