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

✨ Hide traceback for InstanceNotEmpty using Click Exception #930

Merged
merged 5 commits into from
Jan 8, 2025

Conversation

Zethson
Copy link
Member

@Zethson Zethson commented Jan 8, 2025

My earlier solution of

raise InstanceNotEmpty(message) from None

did not quite go far enough and I forgot to test it properly because I got distracted. Here's a new attempt.

Before:

⋊> ~/P/redun-lamin on feature/ruff ⨯ lamin delete test-redun-lamin                                                                                                         (lamindb) 21:08:10
Are you sure you want to delete instance zethson/test-redun-lamin? (y/n) y
Traceback (most recent call last):
  File "/home/zeth/miniconda3/envs/lamindb/bin/lamin", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/zeth/miniconda3/envs/lamindb/lib/python3.11/site-packages/rich_click/rich_command.py", line 367, in __call__
    return super().__call__(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/zeth/miniconda3/envs/lamindb/lib/python3.11/site-packages/click/core.py", line 1161, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/zeth/miniconda3/envs/lamindb/lib/python3.11/site-packages/rich_click/rich_command.py", line 152, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/home/zeth/miniconda3/envs/lamindb/lib/python3.11/site-packages/click/core.py", line 1697, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/zeth/miniconda3/envs/lamindb/lib/python3.11/site-packages/click/core.py", line 1443, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/zeth/miniconda3/envs/lamindb/lib/python3.11/site-packages/click/core.py", line 788, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/zeth/miniconda3/envs/lamindb/lib/python3.11/site-packages/lamin_cli/__main__.py", line 209, in delete
    return delete(instance, force=force)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/zeth/PycharmProjects/lamindb-setup/lamindb_setup/_delete.py", line 102, in delete
    n_objects = check_storage_is_empty(
                ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/zeth/PycharmProjects/lamindb-setup/lamindb_setup/core/upath.py", line 875, in check_storage_is_empty
    raise InstanceNotEmpty(message).with_traceback(tb) from None
  File "/home/zeth/PycharmProjects/lamindb-setup/lamindb_setup/core/upath.py", line 870, in check_storage_is_empty
    raise InstanceNotEmpty(message)
lamindb_setup.core.upath.InstanceNotEmpty: Storage '/home/zeth/PycharmProjects/redun-lamin/docs/test-redun-lamin/.lamindb' contains 6 objects - delete them prior to deleting the instance

After:

⋊> ~/P/redun-lamin on feature/ruff ⨯ lamin delete test-redun-lamin                                                                                                         (lamindb) 21:14:56
Are you sure you want to delete instance zethson/test-redun-lamin? (y/n) y
╭─ Error ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ Storage '/home/zeth/PycharmProjects/redun-lamin/docs/test-redun-lamin/.lamindb' contains 6 objects - delete them prior to deleting the instance                                           │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
         

Signed-off-by: zethson <lukas.heumos@posteo.net>
Signed-off-by: zethson <lukas.heumos@posteo.net>
Signed-off-by: zethson <lukas.heumos@posteo.net>
Signed-off-by: zethson <lukas.heumos@posteo.net>
@Zethson Zethson changed the title ✨Use Click exception for deletion error ✨ Hide traceback for InstanceNotEmpty using Click Jan 8, 2025
@Zethson Zethson changed the title ✨ Hide traceback for InstanceNotEmpty using Click ✨ Hide traceback for InstanceNotEmpty using Click Exception Jan 8, 2025
@Zethson
Copy link
Member Author

Zethson commented Jan 8, 2025

I don't understand this test failure:

def test_init_instance_cloud_aws_us():
        storage = (
            f"s3://lamindb-ci/{os.environ['LAMIN_ENV']}_test/init_instance_cloud_aws_us"
        )
        ln_setup.init(storage=storage, _test=True)
        # run for the second time
        # just loads an already existing instance
        ln_setup.init(storage=storage, _test=True)
        hub = connect_hub_with_auth()
        account = select_account_by_handle(
            handle=ln_setup.settings.instance.owner, client=hub
        )
        instance = select_instance_by_name(
            account_id=account["id"],
            name=ln_setup.settings.instance.name,
            client=hub,
        )
        # test default storage record is correct
        storage_record = select_default_storage_by_instance_id(instance["id"], hub)
>       assert storage_record["root"] == storage
E       TypeError: 'NoneType' object is not subscriptable

@Koncopd do you have any idea?

@Koncopd
Copy link
Member

Koncopd commented Jan 8, 2025

Yes, let me check one thing.

Copy link

github-actions bot commented Jan 8, 2025

@github-actions github-actions bot temporarily deployed to pull request January 8, 2025 20:53 Inactive
@Koncopd
Copy link
Member

Koncopd commented Jan 8, 2025

Personally i don't like importing click here.

@Zethson
Copy link
Member Author

Zethson commented Jan 8, 2025

Personally i don't like importing click here.

Yeah, I didn't like it either but I really like the result. I mean click is installed anyways because people have lamin-cli installed, right? There's no harm in importing it, is there?

@Koncopd
Copy link
Member

Koncopd commented Jan 8, 2025

Ok then.

.gitignore Show resolved Hide resolved
@Zethson Zethson merged commit 130926e into main Jan 8, 2025
11 checks passed
@Zethson Zethson deleted the feature/simpler_deletion_ux branch January 8, 2025 20:59
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.51%. Comparing base (58f4749) to head (dc2a369).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
lamindb_setup/core/upath.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #930      +/-   ##
==========================================
- Coverage   83.53%   83.51%   -0.02%     
==========================================
  Files          43       43              
  Lines        3444     3446       +2     
==========================================
+ Hits         2877     2878       +1     
- Misses        567      568       +1     

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

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.

2 participants