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

frontend: always calculate the snippet indexes from the beginning #113

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

FrostyX
Copy link
Member

@FrostyX FrostyX commented Mar 17, 2024

We have been collecting incorrect indexes. They aren't calculated from the end of the log but rather from the end of the previous node.

I am currently working on fixing our collected data.

@praiskup praiskup requested a review from nikromen March 18, 2024 12:23
@nikromen
Copy link
Member

nikromen commented Mar 19, 2024

looking good so far, I checked the indexes on chromium and firefox.

Could you also please rebase to the current HEAD? since the backend part is not the latest so I want to test it against the latest BE just to be sure

We have been collecting incorrect indexes. They aren't calculated from
the end of the log but rather from the end of the previous node.

I am currently working on fixing our collected data.
This will make our life much easier if we find out that snippet
indexes are incorrect again
@TomasTomecek
Copy link
Collaborator

We have been collecting incorrect indexes. They aren't calculated from the end of the log but rather from the end of the previous node.

I am currently working on fixing our collected data.

does this mean that some of our data may be wrong?

Copy link
Collaborator

@TomasTomecek TomasTomecek left a comment

Choose a reason for hiding this comment

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

I can honestly say that I verified it now correctly works in Chrome. Because I accidentally checked out another PR where it was broken, then I correctly checked out this one and suddenly the indexes were right :)

$ podman exec a79b212b15f8 cat /persistent/results/2024-03-22/copr/7197868/1711110203.json > asdw.json

$ ipython3
Python 3.12.2 (main, Feb  7 2024, 00:00:00) [GCC 14.0.1 20240127 (Red Hat 14.0.1-0)]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.21.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import pathlib

In [2]: import json

In [3]: p = pathlib.Path("./asdw.json")

In [4]: j = json.loads(p.read_text())

In [5]: c = j["logs"]["builder-live.log"]["content"]

In [6]: for s in j["logs"]["builder-live.log"]["snippets"]:\
   ...:   print(f"'{c[s['start_index']:s['end_index']]}'\n'{s['user_comment']}'\n======")
'
Start: chroot in'
'start:'
======
'
E   ModuleNotFoundError: No module named 'telnetli'
'E Module'
======

Those were the exact lines I selected.

@FrostyX++ for spotting it and resolving it so precisely

Copy link
Collaborator

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

I also verified this on Brave 1.64.109 . Without this patch the submitted snippets are broken with this patch it works good.

Thanks for the fix.

P.S.: I'm not able to verify code validity just testing.

Copy link
Collaborator

@jpodivin jpodivin left a comment

Choose a reason for hiding this comment

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

I don't know much about clojure, but from where I sit this seems to be very well done.

@nikromen nikromen marked this pull request as draft March 25, 2024 12:03
@nikromen
Copy link
Member

Thanks everybody for the reviews! It works for the most cases. However, there are 2 issues:

  • 1 issue is small - the indexes are shifted by 2 or 1 - not a big deal, the context still remains
  • 2 issue is however worse - if you scroll around the log and add a few snippets and then delete some of them, the next snippet indexes are wrong - with 100% reproducibility if you add snippet from the very end of the log. I think this may be caused by the first issue (the index overflows due to the indexes shift?)

@TomasTomecek
Copy link
Collaborator

Thanks everybody for the reviews! It works for the most cases. However, there are 2 issues:

  • 1 issue is small - the indexes are shifted by 2 or 1 - not a big deal, the context still remains
  • 2 issue is however worse - if you scroll around the log and add a few snippets and then delete some of them, the next snippet indexes are wrong - with 100% reproducibility if you add snippet from the very end of the log. I think this may be caused by the first issue (the index overflows due to the indexes shift?)

naive thinking: how about disabling removals?

@nikromen
Copy link
Member

as per chat in slack - merging this to release this fix ASAP and deal with the edgecase later - #117

@nikromen nikromen merged commit d63a054 into fedora-copr:main Mar 25, 2024
3 checks passed
@nikromen
Copy link
Member

naive thinking: how about disabling removals?

That would significantly lower the user experience, potentially lowering the feedbacks we get :/ I used this feature several times. In case I'll need to close it and fill it once again (as normal user) I'd probably left it without giving feedback

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.

5 participants