Fix inline completion bug, add test cases #1002

hiCXK · 2024-09-17T18:28:29Z

@krassowski please check if the edits are correct

packages/jupyter-ai/jupyter_ai/tests/completions/test_handlers.py

dlqqq · 2024-09-25T17:31:36Z

Thank you for your contribution! It's been a week since we've last heard from you, so I'm closing this PR due to inactivity. Please feel free to re-open this PR after addressing the review feedback.

krassowski · 2024-09-25T17:43:53Z

Well, to be fair 3 days to address a review is not that much time if people are contributing in free time. CC @ellisonbg

hiCXK · 2024-09-25T17:58:38Z

Working on the issue... will open this again as soon as I resolve it 😃

ellisonbg · 2024-09-25T17:59:15Z

Yeah, I agree with @krassowski. Jupyter doesn't have any formal standards on this, so each repo and subproject tends to handle it differently. At the same time, I think waiting at least a month or more is pretty normal across Jupyter. At times, some repos have even given people much longer (>1 year), but that quickly makes it difficult to navigate the PR queue. I want to be clear here - I am not making any decisions here, just reflecting how Jupyter has handled this in the past.

ellisonbg · 2024-09-25T18:01:31Z

Thanks @hiCXK for your contributions! We do want to go out of our way to welcome and encourage contributors. I will let others reopen the PR, but we look forward to seeing this PR reviewed and merged :-)

krassowski · 2024-09-25T18:16:12Z

Sorry for the direct ping, I just see that many PR reviews include words like "after discussing internally with the team" so when I saw a pattern of closing PRs based on one week threshold I just wanted to raise this with wider team in case if this was also discussed internally :) To be clear, I am not complaining about having internal reviews of features in addition to public reviews, I think this is awesome that you can do this!

I think that it could make sense for this repo to decide on short PR iteration time, but it would be great to have:

some consensus building in a public issue among contributors (like show of hands between 1/2/3 weeks of inactivity)
it documented in CONTRIBUTING.md or a section of the documentation

:)

ellisonbg · 2024-09-25T20:44:56Z

Thanks @krassowski for bringing this up, this is really important. Obviously, we (at AWS) do discuss things internally, but you are right that we shouldn't ever make decisions in that way or even appear to do so. All information and discussions that are used to make decisions about any Jupyter subproject or repo should be public and on GitHub.

In terms of decision making, we are bound to follow the Jupyter Decision Making Guide. However, for smaller repos (at least smaller than Lab, Notebook, Hub, etc.) like Jupyter AI it would be helpful to add information to the CONTRIBUTING.md with a summary of how that is meant to play out in this particular project, to help people understand the rough process and timelines for review, merging, etc.

I know @dlqqq is working hard to keep up with the fast pace of PRs in this repo and want to acknowledge his amazing work to keep the the PR queue moving, this is an underappreciated part of open source work. I can't believe that this repo has already seen almost 500 PRs merged already – or a relatively small effort that is fantastic.

hiCXK · 2024-09-25T20:53:21Z

@dlqqq please re-open the PR. I have corrected the test cases which should now resolve the CI issue.

dlqqq · 2024-09-26T15:41:11Z

@krassowski Yes, I agree. 3 days is definitely too short, and this was a mistake on my end. I was looking at when the PR was opened, not when your review was given. Thank you for calling out my error and the need for community involvement in establishing contributing standards. 👍

(cc @hiCXK) Can open source contributors re-open PRs on their own, or do they require permission from a maintainer to do so? When I closed this PR, I had thought that this would be a harmless way of reducing the number of open PRs, which helps me identify the ones that require my review more quickly. I certainly didn't mean to "shut the door" on contributors who didn't respond within a certain time. Hope this clarifies my intention here. 🤗

dlqqq · 2024-09-26T16:05:53Z

Sorry for the direct ping, I just see that many PR reviews include words like "after discussing internally with the team" so when I saw a pattern of closing PRs based on one week threshold I just wanted to raise this with wider team in case if this was also discussed internally :) To be clear, I am not complaining about having internal reviews of features in addition to public reviews, I think this is awesome that you can do this!

@krassowski Thank you for addressing the "elephant in the room" here directly. I think it's really wonderful that we respect & trust each other enough to be able to discuss these hard questions directly & publicly. I fully acknowledge that we can do more to be transparent & earn trust with the open source community.

Given the project's substantial growth recently, there have been situations where I/we decided outcomes internally instead of seeking consensus in the interest of time, hoping that our conclusion would be generally accepted. However, this doesn't always work (as proven here 😁), and as @ellisonbg called out, this just isn't the right way to run an open source project. I will meet with Brian and apply his feedback on how to make Jupyter AI more open and transparent in its decision-making. This may take some time given that I'm balancing code review, code contribution, and project governance all at once, so please feel free to open an issue or ping me if you feel that progress isn't being made quickly enough.

Again, hope this clarifies things, and thank you for your feedback!

dlqqq

@krassowski (regarding this PR) Can you review this since you're more familiar with the inline completion logic? You should have permissions to approve & merge as a member of the jupyterlab/ org, so feel free to do so if you're able.

hiCXK · 2024-09-26T16:39:59Z

@krassowski Yes, I agree. 3 days is definitely too short, and this was a mistake on my end. I was looking at when the PR was opened, not when your review was given. Thank you for calling out my error and the need for community involvement in establishing contributing standards. 👍

(cc @hiCXK) Can open source contributors re-open PRs on their own, or do they require permission from a maintainer to do so? When I closed this PR, I had thought that this would be a harmless way of reducing the number of open PRs, which helps me identify the ones that require my review more quickly. I certainly didn't mean to "shut the door" on contributors who didn't respond within a certain time. Hope this clarifies my intention here. 🤗

I think the 'reopen' button wasn't visible to me; it might be only visible to members. As for closing this PR, I understood your intentions 🤗 I should have responded while working on it. Thank you.

krassowski · 2024-09-30T14:24:17Z

packages/jupyter-ai/jupyter_ai/tests/completions/test_handlers.py

+    ("```python\n\n\n# load json file\nimport\n```", "# load json file\nimport"),
+    ("```python\n     # load json file\nimport\n```", "# load json file\nimport"),
+    ("```\n# load json file\nimport\n```", "# load json file\nimport"),
+    (" ```\n# load json file\nimport\n```", "# load json file\nimport"),
+    ("``` \n# load json file\nimport\n```", "# load json file\nimport"),
+    (
+        "\n```python\n# load json file\nimport\n```",
+        "# load json file\nimport",
+    ),


These test cases are not representative of the example that I included in #1001. The point was that user types in instruction like:

# load json file

and the model should follow by responding with something like:

\nimport json \nwith open('path-to-file', 'r') as f: \n content = json.load(f)

What want to test is that the initial line has the \n preserved if it is included in the expected_suggestion. I do not think that these test cases test this, instead they repeat the previous tests just with different text content.

@hiCXK does it make sense to you?

It seems I'm having trouble understanding the usage of response and expected_suggestion.
response is the generated text from the mock LLM and expected_suggestion is the required correct text.
So, can this be a valid test case?
("```\nimport json\nwith open('path-to-file','r') as f:```", "\nimport json\nwith open('path-to-file','r') as f:")
This way we can check if the initial line's \n character is preserved.
Please suggest a test case if my approach is incorrect and correct my approach 🙏

Yes, something along these lines. Sometimes it will include the comment prefix sometimes it won't do two cases are needed here

Ok, so i am adding these test cases:

("```\n# load json file\nimport json\nwith open('path-to-file','r') as f:```", "\n# load json file\nimport json\nwith open('path-to-file','r') as f:"), ("```\nimport json\nwith open('path-to-file','r') as f:```", "\nimport json\nwith open('path-to-file','r') as f:")

are these correct? comment prefix is included. We can also shorten them up because we only have to check the preservation of first \n character

("```\n# load json file\n```", "\n# load json file"), ("```\ndef function():\n pass\n```", "\ndef function():\n pass"),

Also can you elaborate more on inclusion of comment prefix if my solution is incorrect. I have also gone through the community forum and documentation to find more about this.

The proposed shortened tests are fine here. Ideally we would have a separate test which checks that:

response = "```\n# load json file\nimport json\nwith open('path-to-file','r') as f:```"

generated for prefix:

# load json file

gives

result = "\nimport json\nwith open('path-to-file','r') as f:"

To check the code in line 46.

Although the test cases seem correct this time and the function post_process_suggestion is correct, the tests cases are failing unit tests again. There are AssertionError for each test case i added. \n characters are still being stripped off despite changing the function. I must be missing something.

Please suggest the way to resolve them.

for more information, see https://pre-commit.ci

krassowski reviewed Sep 22, 2024

View reviewed changes

packages/jupyter-ai/jupyter_ai/tests/completions/test_handlers.py Outdated Show resolved Hide resolved

dlqqq added the bug Something isn't working label Sep 23, 2024

dlqqq closed this Sep 25, 2024

dlqqq reopened this Sep 26, 2024

dlqqq reviewed Sep 26, 2024

View reviewed changes

krassowski requested changes Sep 30, 2024

View reviewed changes

hiCXK force-pushed the main branch from 4f1b012 to 619a92f Compare October 3, 2024 15:46

hiCXK and others added 4 commits October 4, 2024 22:15

Fix inline completion bug, add test cases

927ede1

[pre-commit.ci] auto fixes from pre-commit.com hooks

ef2c976

for more information, see https://pre-commit.ci

Fix test cases

71f7517

[pre-commit.ci] auto fixes from pre-commit.com hooks

3746241

for more information, see https://pre-commit.ci

hiCXK force-pushed the main branch from 619a92f to 3746241 Compare October 4, 2024 16:45

hiCXK and others added 2 commits October 4, 2024 22:58

changed the unit test cases

9738c5a

[pre-commit.ci] auto fixes from pre-commit.com hooks

0341ee5

for more information, see https://pre-commit.ci

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inline completion bug, add test cases #1002

Fix inline completion bug, add test cases #1002

hiCXK commented Sep 17, 2024 •

edited

Loading

dlqqq commented Sep 25, 2024

krassowski commented Sep 25, 2024

hiCXK commented Sep 25, 2024

ellisonbg commented Sep 25, 2024

ellisonbg commented Sep 25, 2024

krassowski commented Sep 25, 2024 •

edited

Loading

ellisonbg commented Sep 25, 2024 •

edited by dlqqq

Loading

hiCXK commented Sep 25, 2024

dlqqq commented Sep 26, 2024

dlqqq commented Sep 26, 2024

dlqqq left a comment

hiCXK commented Sep 26, 2024

krassowski Sep 30, 2024

hiCXK Sep 30, 2024

krassowski Sep 30, 2024 •

edited

Loading

hiCXK Sep 30, 2024 •

edited

Loading

krassowski Oct 3, 2024

hiCXK Oct 4, 2024

Fix inline completion bug, add test cases #1002

Are you sure you want to change the base?

Fix inline completion bug, add test cases #1002

Conversation

hiCXK commented Sep 17, 2024 • edited Loading

dlqqq commented Sep 25, 2024

krassowski commented Sep 25, 2024

hiCXK commented Sep 25, 2024

ellisonbg commented Sep 25, 2024

ellisonbg commented Sep 25, 2024

krassowski commented Sep 25, 2024 • edited Loading

ellisonbg commented Sep 25, 2024 • edited by dlqqq Loading

hiCXK commented Sep 25, 2024

dlqqq commented Sep 26, 2024

dlqqq commented Sep 26, 2024

dlqqq left a comment

Choose a reason for hiding this comment

hiCXK commented Sep 26, 2024

krassowski Sep 30, 2024

Choose a reason for hiding this comment

hiCXK Sep 30, 2024

Choose a reason for hiding this comment

krassowski Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

hiCXK Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

krassowski Oct 3, 2024

Choose a reason for hiding this comment

hiCXK Oct 4, 2024

Choose a reason for hiding this comment

hiCXK commented Sep 17, 2024 •

edited

Loading

krassowski commented Sep 25, 2024 •

edited

Loading

ellisonbg commented Sep 25, 2024 •

edited by dlqqq

Loading

krassowski Sep 30, 2024 •

edited

Loading

hiCXK Sep 30, 2024 •

edited

Loading