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

🚸 Improve UX of db$track() and db$finish() #120

Merged
merged 10 commits into from
Dec 1, 2024
Merged

Conversation

lazappi
Copy link
Collaborator

@lazappi lazappi commented Nov 29, 2024

Improve UX for db$track() and db$finish().

db$track()

Before:

> db$track(path = "./my-analysis.Rmd")
Got UID "92h33eSG2zNz0000" for path alex-rnb.Rmd. Run this function with `transform = "92h33eSG2zNz0000"` to track this path.

After:

> db$track()
To track this notebook, run: db$track("92h33eSG2zNz0000")

db$finish()

Before:

> db$finish()
Error in py_call_impl(callable, call_args$unnamed, call_args$named) : 
  lamindb.core.exceptions.NotebookNotSaved: Please save the notebook in RStudio (shortcut `CMD + s`) within 2 sec before calling `db$finish()`
Run `reticulate::py_last_error()` for details.

After:

> db$finish()
NotebookNotSaved: Please save the notebook in RStudio (shortcut `CMD + s`) within 2 sec before calling `db$finish()`

Implementation

  • Add a detect_path() function
  • Use this function in Instance$track() to attempt to detect which path to track

Needs:

Resolves:

@lazappi
Copy link
Collaborator Author

lazappi commented Nov 29, 2024

@falexwolf I think this should work but I'm running into an issue on the Python side. If the file I already exists I get a prompt:

→ Ready to re-run? (y/n)

Which causes rendering to get stuck because it doesn't get a response. Is there away to avoid this?

Also, the error message I was using to get the UID seems to have changed. The error doesn't include the UID anymore, instead it is outputted as a separate message and the error just says "Please follow the instructions". Was this a deliberate change? I'm not sure if I can capture the Python message to get the UID but I can look into it if this is how it's supposed to work.

@falexwolf
Copy link
Member

→ Ready to re-run? (y/n)

I see. The transform type of an .Rmd and .qmd is now notebook and no longer script, and hence this fires:

https://github.com/laminlabs/lamindb/blob/6df9aa98ebbd37bb8710524124d859346349e11f/lamindb/core/_context.py#L93-L103

I will fix this right away on the autoknit branch.

Also, the error message I was using to get the UID seems to have changed.

This is the same issue as the one above.

2 min!

@falexwolf
Copy link
Member

falexwolf commented Nov 29, 2024

It's fixed here (now checks for is_run_from_ipython instead of transform.type == "notebook")

laminlabs/lamindb@66fd233

On the autoknit branch of lamindb:

@lazappi
Copy link
Collaborator Author

lazappi commented Nov 29, 2024

Thanks! It's doing what I expected now.

@lazappi lazappi marked this pull request as ready for review November 29, 2024 10:26
@falexwolf
Copy link
Member

I haven't gotten found the time to install RStudio and understand how it writes files etc. but I just found a stupid bug for the timestamp check. It's fixed here:

Can you please try to call db$finish() on a non-saved notebook? This should trigger the error I'm commenting on below because the timestamp of the report file isn't recent enough:

Maybe this fixes everything already. 😅

@falexwolf falexwolf changed the title Automatically detect paths for tracking 🚸 Automatically detect the path for db$track() Nov 29, 2024
@lazappi
Copy link
Collaborator Author

lazappi commented Nov 29, 2024

We can probably handle the error message better but I think the timestamp check works now 🎉!

I also got this prompt:

> db$finish()
You are about to overwrite existing source code (hash 'wHdxw0-8ad5J_Ivta848CQ') for Transform('eusxxmRiC1YA0000'). Proceed? (y/n) y
! Please re-run `ln.track()` to make a new version

But it worked after running db$track() again https://lamin.ai/laminlabs/lamindata/transform/eusxxmRiC1YA0000.

If I try running it a second time I keep getting the NotebookNotSaved error though (even if I save everything). Maybe because the code hasn't changed?

@lazappi lazappi requested a review from rcannood November 29, 2024 11:11
@falexwolf
Copy link
Member

We can probably handle the error message better but I think the timestamp check works now 🎉!

Ok, this is good!

I also got this prompt:
...

Hm. This is strange and has to be resolved. 🤔

I'm wondering whether that's a consequence of your particular setup. Upon calling db$track() the transform$source_code is not saved and no hash is computed.

Saving source code and computing its hash only happens upon db$finish() or lamin save my-analysis.Rmd.

If you're working with a transform uid that you used before and for which you already saved source code, then you'll get this

You are about to overwrite existing source code (hash 'wHdxw0-8ad5J_Ivta848CQ') for Transform('eusxxmRiC1YA0000'). Proceed? (y/n) y

This should then simply overwrite the existing source code without asking you to make a new version.

Can you clarify whether this is what happened?

If I try running it a second time I keep getting the NotebookNotSaved error though (even if I save everything). Maybe because the code hasn't changed?

Calling db$finish() a second time? Hm, the timestamp check only checks for the 2sec time window. I can think more about what might trigger this.

Copy link
Collaborator

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

Code diff looks good to me

  • Could you add an entry to the changelog?

@falexwolf
Copy link
Member

falexwolf commented Nov 29, 2024

Could you add an entry to the changelog?

From my perspective, you guys wouldn't need to maintain a changelog manually because it's auto-generated:

@falexwolf
Copy link
Member

Can we merge and release package 0.3.0 on Monday morning?

This is already a ton better and we want this out so that anybody who tries it will have the new version next week.

@falexwolf
Copy link
Member

If there is time on Monday morning, ironing out the remaining questions here would of course be great:

@falexwolf falexwolf changed the title 🚸 Automatically detect the path for db$track() 🚸 Improve UX of db$track() and db$finish() Dec 1, 2024
@falexwolf
Copy link
Member

falexwolf commented Dec 1, 2024

Ok, I spent the past 5 hours playing with RStudio and the typical workflows of iterating on notebooks, getting notebook versions from the hub, re-running them, etc. ☺️

It's now smooth for most cases. 🎉 -- It required all kinds of small tweaks to the lamindb code though. They're all here:

There is one edge case left:

This is for another time.

We need to get the release out now and say it depends on lamindb 0.77.1: pip install lamindb>=0.77.1

@falexwolf falexwolf merged commit aa99e06 into main Dec 1, 2024
7 checks passed
@falexwolf falexwolf deleted the issue-115/detect-path branch December 1, 2024 22:36
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.

3 participants