✨ Allow login with new API key

[Koncopd]

No description provided.

[github-actions]

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

[codecov]

Codecov Report

Attention: Patch coverage is 93.42105% with 5 lines in your changes missing coverage. Please review.

Project coverage is 83.41%. Comparing base (bc17873) to head (d164dd3).

Files with missing lines	Patch %	Lines
lamindb_setup/_setup_user.py	90.62%	3 Missing ⚠️
lamindb_setup/core/_hub_core.py	92.85%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #840      +/-   ##
==========================================
+ Coverage   83.32%   83.41%   +0.09%     
==========================================
  Files          42       42              
  Lines        3250     3299      +49     
==========================================
+ Hits         2708     2752      +44     
- Misses        542      547       +5     

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

[Koncopd]

Ok, this adds
login(api_key="some_api_key")
Loads a user with API key and allows to renew an access token with the stored API key.

[Koncopd]

@falexwolf are you fine with this?

[falexwolf]

This looks great except that I don't fully get the .env file management. 🤔

The main issue is that his will raise an error for everybody when it goes live. We have the same issue for the instance env file where we'll add the api_url.

I think it'd be worthwhile to no longer use pydantic to validate the env file but instead just open the text file and parse it so that doesn't destroy everybody's workflow.

[Koncopd]

I will try to fix this behavior here.

[falexwolf]

We're already not using pydantic to write settings.

So, you should be able to create a read_settings function similar to the below and remove pydantic from the dependencies here.

lamindb-setup/lamindb_setup/core/_settings_save.py

Lines 34 to 65 in bc17873

	def save_settings(
	settings: Any,
	settings_file: Path,
	type_hints: dict[str, Any],
	prefix: str,
	):
	with open(settings_file, "w") as f:
	for store_key, type in type_hints.items():
	if type == Optional[str]:
	type = str
	if type == Optional[bool]:
	type = bool
	if "__" not in store_key:
	if store_key == "model_config":
	continue
	if store_key == "storage_root":
	value = settings.storage.root_as_str
	elif store_key == "storage_region":
	value = settings.storage.region
	else:
	if store_key in {"db", "schema_str", "name_", "uuid", "id"}:
	settings_key = f"_{store_key.rstrip('_')}"
	else:
	settings_key = store_key
	value = getattr(settings, settings_key)
	if value is None:
	value = "null"
	elif isinstance(value, UUID):
	value = value.hex
	else:
	value = type(value)
	f.write(f"{prefix}{store_key}={value}\n")

[Koncopd]

@falexwolf this is actually enough to avoid the validation error.
Do you think the rewrite of settings load is still needed? It is not hard to do, especially via dotenv_values like here, i just think it is reasonable to offload loading and validation logic to pydantic-settings.

[falexwolf]

Ok! If users don't run into errors that's alright!

[falexwolf]

Another thing: This should be aware of an environement variable "LAMIN_API_KEY" which then allows to bypass things.

Exactly like wandb do it: https://docs.lamin.ai/wandb

[Koncopd]

I thought that "LAMIN_API_KEY" should be for the CLI. Should ln_setup.login() try to read the env variable also?

[Koncopd]

I would describe it like this. I don't want to describe the old key as "legacy" while the new approach is still tested.

[falexwolf]

Agreed! You can call the new key "Beta API key" and the old key "API key".

[falexwolf]

And at some point we switch it to "Legacy API key" and "API key".

[falexwolf]

I thought that "LAMIN_API_KEY" should be for the CLI. Should ln_setup.login() try to read the env variable also?

Yes. The CLI shouldn't add additional logic. If people call ln.setup.login() without args this should behave in the same way as lamin login.

[falexwolf]

Well I agree that the CLI should be responsible for the logic around the input() prompt.

[Koncopd]

Yes, but CLI should still check if "LAMIN_API_KEY" is present to avoid calling input.

[Koncopd]

@falexwolf added the logic for "LAMIN_API_KEY"

[falexwolf]

Great!

[Koncopd]

Ok, merging this then.