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

Annotate the version of an item #1041

Open
4 tasks done
tuscland opened this issue Jan 3, 2025 · 16 comments
Open
4 tasks done

Annotate the version of an item #1041

tuscland opened this issue Jan 3, 2025 · 16 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@tuscland
Copy link
Member

tuscland commented Jan 3, 2025

Is your feature request related to a problem? Please describe.

As a data scientist, I want to annotate the specific version of an item because it helps me keep track of what is happening.

As a data scientist, I want to update or delete the note I have previously created.

As a data scientist, the notes I create from the Python API must be visible in the UI.

Describe the solution you'd like

Python API

The value of the note kwarg is some Markdown text.

Add an note:

skore.put(
    "feat_imps",
    fig,
    note="added AggJoiner with max_observation=2"
)

More programmatic CRUD:

item_version = skore.get_item_versions("feat_imps")[0]

# If there is no note, it creates a note, otherwise update the note
item_version.set_note("set frunobulax level at 11")
note = item_version.get_note()

item_version.delete_note()

UI

Let the user interact with an item version:

  • Create / Update a note using Markdown text.
  • Delete the note.
  • View the note of an item (last version)
  • View the note of a specific item version

Describe alternatives you've considered, if relevant

No response

Additional context

Related to #889

@tuscland tuscland added enhancement New feature or request epic This issue represents major product increments labels Jan 3, 2025
@tuscland tuscland added this to the Skore 0.7 milestone Jan 7, 2025
@tuscland tuscland removed the epic This issue represents major product increments label Jan 8, 2025
@augustebaum
Copy link
Contributor

Without put_item it seems harder to achieve the set_note behaviour, as Items are just dataclasses with no link to a Project.

@rouk1
Copy link
Contributor

rouk1 commented Jan 8, 2025

To me, we need to introduce IDs for items. They could be a simple string interpolation {name}-{counter}.
Then note may be a new entity with a foreign key. Even without a proper db engine we can do this in repositories.

@augustebaum
Copy link
Contributor

Yes, then set_note could be a method of Project instead:

project.put("my_key", 3)

# Annotates the latest version by default
project.set_note("my_key", "this is 3 because 3 is good")

# Annotates version 10
project.set_note("my_key-10", "this is 3 because 3 is good")

@thomass-dev
Copy link
Collaborator

thomass-dev commented Jan 10, 2025

@augustebaum , we shouldn't add the version in the key, prefer a dedicated parameter.

In your example, imagine you have two items in your project with keys my_key (10 versions) and my_key-10 (1 version).
How to differentiate the version 10 of the my_key item from the my_key-10 item?

I would prefer something like:

 class Project:
     def set_note(self, key, note, *, version=-1):
         ...

Edit:

We can maybe add a new parameter to the put function too put(key, value, *, note=None).

@MarieS-WiMLDS
Copy link
Contributor

agree with Thomas's solution, I like it, it seems fluid. I also like to be able to put a note at first and also afterwards.

@MarieS-WiMLDS MarieS-WiMLDS removed their assignment Jan 10, 2025
augustebaum pushed a commit that referenced this issue Jan 14, 2025
This is for several reasons:
- it is not as explicit as several simple `put` calls: the mechanic
is ambiguous with regards to atomicity (if a key-value pair is
invalid, does it make the whole operation fail?)
- the mechanic makes it complicated to add options to `put`,
e.g. the `note` option proposed in
#1041
or the `display_as` option proposed in
#1045 (comment).
augustebaum pushed a commit that referenced this issue Jan 14, 2025
This is for several reasons:
- it is not as explicit as several simple `put` calls: the mechanic
is ambiguous with regards to atomicity (if a key-value pair is
invalid, does it make the whole operation fail?)
- the mechanic makes it complicated to add options to `put`,
e.g. the `note` option proposed in
#1041
or the `display_as` option proposed in
#1045 (comment).
augustebaum pushed a commit that referenced this issue Jan 14, 2025
This is for several reasons:
- it is not as explicit as several simple `put` calls: the mechanic
is ambiguous with regards to atomicity (if a key-value pair is
invalid, does it make the whole operation fail?)
- the mechanic makes it complicated to add options to `put`,
e.g. the `note` option proposed in
#1041
or the `display_as` option proposed in
#1045 (comment).
thomass-dev pushed a commit that referenced this issue Jan 14, 2025
This is for several reasons:
- it is not as explicit as several simple `put` calls: the mechanic
is ambiguous with regards to atomicity (if a key-value pair is
invalid, does it make the whole operation fail?)
- the mechanic makes it complicated to add options to `put`,
e.g. the `note` option proposed in
#1041
or the `display_as` option proposed in
#1045 (comment).
thomass-dev pushed a commit that referenced this issue Jan 14, 2025
New methods have been added to `Project`:

```python
# The default is to act on the latest version
project.set_note(key, message, version=-1)

project.get_note(key, message, version=-1)

project.delete_note(key, message, version=-1)
```

To this end,
- `Item` now has an additional attribute, `note`, which is a `str |
None`
- `ItemRepository` has similar methods

The note related methods fail with a KeyError when the key-version
combination does not exist, but:
- `get_note` returns `None` if the note is `None` (no error)
- `delete_note` never errors if the key-version combination exists (even
if the note is already `None`, it will be set to `None` again)

`set_note` fails with a TypeError if `key` or `message` is not a string.

Addresses part of #1041
@augustebaum augustebaum self-assigned this Jan 14, 2025
@MarieS-WiMLDS
Copy link
Contributor

The UI has been defined :)!
It can be found in this figma: https://www.figma.com/design/Qzjyxv0lPDU2kKtsH9dcp3/Skore---Workstation---High-fidelity-mock-ups---Q3?node-id=5404-14131&t=TZMMlaSkT983MfKM-4

@rouk1 rouk1 self-assigned this Jan 15, 2025
augustebaum pushed a commit that referenced this issue Jan 16, 2025
Part of #1041 

- [x] upgrade dependencies (eslint 9 \o/)
- [x] new icons
- [x] minimal skinnable rich text editor
- [x] expose python's `set_note` as a JSON API 
- [x] plug the API in the frontend

## UI preview


https://github.com/user-attachments/assets/33cdf005-9ddd-42db-8059-c84c6a4a2433

## implementation choices

Finding a good and not too opinionated rich text editor is no easy job.
After struggling with a few libs I decided that rolling a minimal editor
may do the job (at least for now).

The RichTextEditor component (probably badly named) tries to:
- help user with basic markdown syntax
- keep interactions button outside of it for best skinnability

In a future work we may move to something more powerful.
@augustebaum
Copy link
Contributor

With #1082 merged I believe a first iteration of this issue is done! Please review and close if you agree @MarieS-WiMLDS @sylvaincom

@sylvaincom
Copy link
Contributor

Many thanks team! All good on my side, I'm letting @MarieS-WiMLDS give her approval as well

@MarieS-WiMLDS
Copy link
Contributor

MarieS-WiMLDS commented Jan 17, 2025

Hello!

Starting to play with the notes, I have a couple of questions.

I can't wait to show it to people who asked for it :)!

Q1

project.put("first_item 2", "some text here to change ", note="a note") # is ok
project.put("first_item 2", "some text here to change ", "a note") # is not ok

Why is the second option not ok? Also, why does it render this error, I don't understand why it tells me I give 4 arguments while I read only 3? Finally, the documentation of put doesn't seem to be updated, is it something you are planning to do later?

Image

Q2

Image

It was a bit difficult to me to understand how to save the note. I clicked on the screen outside of the note, but it didn't close the editing window. I'm not sure it should, be willing to hear your opinion about it.

Q3

When I write a simple note, like "blblbl", when I fetch it using:

item_v = project.get_item_versions("first_item 2")[0]
item_v.note
# outputs 'blblbl\n'. Where does \n comes from?
Capture.video.du.17-01-2025.09.38.57.webm

Q4

more a remark; it looks like we haven't been clear in the specs, sometimes we talk about a message, sometimes a note. everything is not consistant. --> let's harmonize everything on "note".

Image

@augustebaum
Copy link
Contributor

augustebaum commented Jan 17, 2025

Great feedback @MarieS-WiMLDS

Re Q1: The second option is not ok because when you start adding a lot of arguments to a function, passing arguments by keyword is less ambiguous (otherwise the user might be confused as to which argument is the value and which is the note).

Re Q2: This was discussed with @rouk1 and I agree with you -- I thought it was something we could deal with later. Currently your options are all keyboard-based (Esc, Shift+Enter or Alt+Enter)

Re Q3: I can see in the video that you pressed Enter before saving the note, that would be why. Maybe we can strip those @rouk1 EDIT: When I press Shift+Enter, it adds a newline and then saves

Re Q4: You're right, I am only noticing it now. See #1143

@sylvaincom
Copy link
Contributor

sylvaincom commented Jan 17, 2025

Re Q2: as in a markdown cell in Jupyter notebook, you do Shift+Enter

@rouk1
Copy link
Contributor

rouk1 commented Jan 17, 2025

It was a bit difficult to me to understand how to save the note. I clicked on the screen outside of the note, but it didn't close the editing window. I'm not sure it should, be willing to hear your opinion about it.

I agree that the question could be asked. Behind the scene your note is saved at each keystroke, hence no need to focus out or to click save (à la notion). I suppose this is fine.

I'll open a PR to handle "click outside" and trimming.

@MarieS-WiMLDS
Copy link
Contributor

re Q1: ok, I understand why it's not a good idea! However I still don't understand the error: it tells me I gave 4 arguments, while I only gave 3. Is there something we can do about it? Also, the documentation of put is not updated yet, do you have a reminder somewhere to do it?

@auguste-probabl
Copy link
Contributor

auguste-probabl commented Jan 20, 2025

I still don't understand the error: it tells me I gave 4 arguments, while I only gave 3. Is there something we can do about it?

That's because it also counts self, as put is a method of project. That's something to do with how Python works, I don't thnk we can mitigate this... @thomass-dev do you agree?

the documentation is not updated yet

@thomass-dev #1052 should update the Project docs indeed. As for the public docs, they haven't been built in 1 week because OpenML is currently down, which means our examples don't run. We were hoping this would be resolved by now... @thomass-dev and I will start work on a fix shortly.

@MarieS-WiMLDS
Copy link
Contributor

thanks for the answers, and for the "click outside" addition.

Our last element to tackle before closing this issue is the saving of the note. As @auguste-probabl said, When I press Shift+Enter, it adds a newline and then saves.

@rouk1
Copy link
Contributor

rouk1 commented Jan 20, 2025

thanks for the answers, and for the "click outside" addition.

Our last element to tackle before closing this issue is the saving of the note. As @auguste-probabl said, When I press Shift+Enter, it adds a newline and then saves.

Done in #1167

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants