♻️ Overhaul `init` for more convenient use in laminhub #857

Koncopd · 2024-09-18T16:02:41Z

https://github.com/laminlabs/laminhub/issues/1271

This moves all internal utility arguments to kwargs in lamindb_setup.connect.
Adds 2 additional arguments: _user and _write_settings.

_user allows to pass user settings (with access_token) to connect and they are used instead of lamindb_setup.settings.user.
_write_settings when set to False forces connect not to write any settings env files. It is set to True by default for now.

I believe this addresses point 1-2 in the linked issue.

Koncopd · 2024-09-18T16:03:45Z

lamindb_setup/_connect_instance.py

+
+    kwargs.get("access_token", None)
+    _raise_not_found_error = kwargs.get("_raise_not_found_error", True)
+    _test = kwargs.get("_test", False)


@falexwolf are you fine with moving all internal arguments to kwargs in connect and init?

I thought about this again and what I'd prefer is to have a properly typed signature of a function called connect_internal() that's wrapped by the user-facing connect().

This way we eliminate **kwargs. We really shouldn't have them anywhere unless those places where we can't come up with a different solution.

The user-facing connect() will be super minimal and simple after also incorporating this change: #862

And then we'll also be ready to attack this: https://github.com/laminlabs/pfizer-lamin-usage/issues/25

I agree, i just think that this probably to do in another PR, it requires a lot of other things, its own tests etc that i don't want to do here.

github-actions · 2024-09-18T16:08:32Z

🚀 Deployed on https://66f2a00b9a9c14f89bba2696--lamindb-setup-htry.netlify.app

codecov · 2024-09-18T16:09:32Z

Codecov Report

Attention: Patch coverage is 80.48780% with 16 lines in your changes missing coverage. Please review.

Project coverage is 83.52%. Comparing base (7f2bbb1) to head (b7c8912).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
lamindb_setup/_connect_instance.py	70.83%	7 Missing ⚠️
lamindb_setup/_init_instance.py	80.00%	6 Missing ⚠️
lamindb_setup/core/cloud_sqlite_locker.py	50.00%	2 Missing ⚠️
lamindb_setup/core/_settings_instance.py	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #857      +/-   ##
==========================================
+ Coverage   83.45%   83.52%   +0.07%     
==========================================
  Files          43       43              
  Lines        3367     3400      +33     
==========================================
+ Hits         2810     2840      +30     
- Misses        557      560       +3

Flag	Coverage Δ
	`83.52% <80.48%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Koncopd · 2024-09-23T14:08:57Z

@falexwolf @fredericenard this is ready for a review.

falexwolf · 2024-09-23T14:33:49Z

lamindb_setup/_connect_instance.py

        except InstanceNotFoundError as e:
            if _raise_not_found_error:
                raise e
            else:
                return "instance-not-found"
        if isinstance(isettings, str):
            return isettings
+        # after we checked that isettings is not a string


The comment here is incomprehensible to me just because the wording is poor.

lamindb_setup/core/_hub_core.py

falexwolf · 2024-09-23T14:38:09Z

lamindb_setup/core/_hub_core.py

+    client: Client,
+    ssettings: StorageSettings,
+    auto_populate_instance: bool,
+    created_by: str | None = None,


Can you call this created_by_uuid and type it with UUID instead of str? The conversion via .hex should come right before hitting the REST API so the the code internals are all properly typed.

Can i leave it created_by, but redo the typing? Because i think it is just redundant to do both, i.e. renaming to created_by_uuid and typing with UUID?

Now using UUID.

falexwolf · 2024-09-23T14:39:24Z

lamindb_setup/core/_hub_core.py



-def _init_instance(isettings: InstanceSettings, client: Client) -> None:
+def _init_instance(
+    client: Client, isettings: InstanceSettings, account_id: str | None = None


Well, I guess it works if client is the first argument. Then let's keep it this way.

But account_id has to be a UUID not a str!

falexwolf · 2024-09-23T14:41:03Z

lamindb_setup/core/_settings_instance.py

-        shutil.copy2(filepath, current_instance_settings_file())
-        # persist under settings class for same session reference
-        # need to import here to avoid circular import
+    def _persist(self, write: bool = True) -> None:


Can you add a docstring or make this more explicit by calling the arg save_to_disk or similar?

falexwolf

Great, Sergei! Only very small changes requested from my side.

Koncopd · 2024-09-23T15:00:12Z

Thx, will address the comments.

Koncopd marked this pull request as draft September 18, 2024 16:02

Koncopd commented Sep 18, 2024

View reviewed changes

github-actions bot temporarily deployed to pull request September 18, 2024 16:08 Inactive

github-actions bot temporarily deployed to pull request September 18, 2024 16:55 Inactive

Koncopd force-pushed the rework_init branch from aa9d501 to 02bf40b Compare September 19, 2024 08:14

github-actions bot temporarily deployed to pull request September 19, 2024 08:20 Inactive

github-actions bot temporarily deployed to pull request September 19, 2024 09:10 Inactive

github-actions bot temporarily deployed to pull request September 19, 2024 10:53 Inactive

github-actions bot temporarily deployed to pull request September 19, 2024 16:15 Inactive

github-actions bot temporarily deployed to pull request September 19, 2024 17:19 Inactive

github-actions bot temporarily deployed to pull request September 20, 2024 08:22 Inactive

github-actions bot temporarily deployed to pull request September 20, 2024 08:42 Inactive

github-actions bot temporarily deployed to pull request September 20, 2024 10:59 Inactive

github-actions bot temporarily deployed to pull request September 20, 2024 11:32 Inactive

Koncopd marked this pull request as ready for review September 20, 2024 12:31

Koncopd requested review from falexwolf and fredericenard September 23, 2024 14:09

falexwolf reviewed Sep 23, 2024

View reviewed changes

lamindb_setup/core/_hub_core.py Show resolved Hide resolved

falexwolf reviewed Sep 23, 2024

View reviewed changes

falexwolf requested changes Sep 23, 2024

View reviewed changes

Koncopd added 3 commits September 24, 2024 10:35

move internal arguments to kwargs

6f79f77

allow passing access_token in connect

e31c8e0

use access_token in validate_init_args

25f1171

Koncopd added 13 commits September 24, 2024 10:35

add access_token to init

144aa1d

allow avoiding settings writes on connect and init

b190953

build around passing user instead of access_token in init, connect

7648cee

fix

7210511

correct sqlite upload msg on init

ae23f27

fix write settings

b8502b6

do not write no_lnschema_bionty_file

55da389

add and use _locker_user in InstanceSettings

402ad12

test

7cf64ca

additional check

8d59c9c

edit

e1102fa

fix

3f906f5

Use UUID

5087b7f

Koncopd force-pushed the rework_init branch from 166ca6d to 5087b7f Compare September 24, 2024 09:01

fix

9dbffed

falexwolf changed the title ~~♻️ Rework init for use in laminhub~~ ♻️ Overhaul init for use in laminhub Sep 24, 2024

falexwolf changed the title ~~♻️ Overhaul init for use in laminhub~~ ♻️ Overhaul init for more convenient use in laminhub Sep 24, 2024

fix

828b2d3

github-actions bot temporarily deployed to pull request September 24, 2024 09:37 Inactive

falexwolf self-requested a review September 24, 2024 10:10

falexwolf approved these changes Sep 24, 2024

View reviewed changes

docs for isettings._persist

57aed4b

github-actions bot temporarily deployed to pull request September 24, 2024 11:05 Inactive

check locker

b7c8912

github-actions bot temporarily deployed to pull request September 24, 2024 11:18 Inactive

Koncopd merged commit 6498329 into main Sep 24, 2024
12 of 13 checks passed

Koncopd deleted the rework_init branch September 24, 2024 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ Overhaul `init` for more convenient use in laminhub #857

♻️ Overhaul `init` for more convenient use in laminhub #857

Koncopd commented Sep 18, 2024 •

edited

Loading

Koncopd Sep 18, 2024

falexwolf Sep 23, 2024

falexwolf Sep 24, 2024

falexwolf Sep 24, 2024

falexwolf Sep 24, 2024

Koncopd Sep 24, 2024

falexwolf Sep 24, 2024

github-actions bot commented Sep 18, 2024 •

edited

Loading

codecov bot commented Sep 18, 2024 •

edited

Loading

Koncopd commented Sep 23, 2024

falexwolf Sep 23, 2024

Koncopd Sep 24, 2024

falexwolf Sep 23, 2024

Koncopd Sep 24, 2024

Koncopd Sep 24, 2024

falexwolf Sep 23, 2024

Koncopd Sep 24, 2024

falexwolf Sep 23, 2024

Koncopd Sep 24, 2024

falexwolf left a comment

Koncopd commented Sep 23, 2024

♻️ Overhaul init for more convenient use in laminhub #857

♻️ Overhaul init for more convenient use in laminhub #857

Conversation

Koncopd commented Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2024 • edited Loading

codecov bot commented Sep 18, 2024 • edited Loading

Codecov Report

Koncopd commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

falexwolf left a comment

Choose a reason for hiding this comment

Koncopd commented Sep 23, 2024

♻️ Overhaul `init` for more convenient use in laminhub #857

♻️ Overhaul `init` for more convenient use in laminhub #857

Koncopd commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

codecov bot commented Sep 18, 2024 •

edited

Loading