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.

Sign up for GitHub

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

Jump to bottom

[JS] fix: streaming - citations, feedback loop, and duplicate typing activity #2185

Merged
merged 8 commits into from
Jan 6, 2025
Merged

[JS] fix: streaming - citations, feedback loop, and duplicate typing activity #2185

merged 8 commits into from
Jan 6, 2025

Conversation

lilyydu
Copy link
Contributor

@lilyydu lilyydu commented Nov 14, 2024 •
edited
Loading

Linked issues

closes: #minor

Details

  • Citations is now enabled during streaming, and for the final message.
  • Added in the new updates to feedback loop
  • Also resolved a few bugs.

image

Change details

Bugs addressed:

  • instead of per chunk, AOAI is sending back citations in the very first chunk, which does not contain any text. however, this was never being saved because we only save and stream the chunk if there is text
  • the citations returned is for the full rendered message, but we were filtering the citations based on the current text, so it gets wiped out
  • timeout needs to be in milliseconds instead of seconds -> this forces us to increase our test timeout to 5 seconds
    image
  • duplicate typing activity was being sent

Attestation Checklist

  • My code follows the style guidelines of this project

  • I have checked for/fixed spelling, linting, and other errors

  • I have commented my code for clarity

  • I have made corresponding changes to the documentation (updating the doc strings in the code is sufficient)

  • My changes generate no new warnings

  • I have added tests that validates my changes, and provides sufficient test coverage. I have tested with:

    • Local testing
    • E2E testing in Teams

  • New and existing unit tests pass locally with my changes

aacebo
aacebo previously approved these changes Nov 14, 2024
View reviewed changes
@lilyydu lilyydu changed the title [JS] fix: citations & streaming [JS] fix: streaming - citations and duplicate typing activity Nov 15, 2024
@lilyydu lilyydu mentioned this pull request Nov 25, 2024
Merged
1 task
corinagum
corinagum previously approved these changes Nov 25, 2024
View reviewed changes
@lilyydu lilyydu mentioned this pull request Nov 25, 2024
Merged
1 task
@lilyydu lilyydu changed the title [JS] fix: streaming - citations and duplicate typing activity DO NOT MERGE [JS] fix: streaming - citations and duplicate typing activity Dec 3, 2024
@lilyydu lilyydu requested a review from heyitsaamir as a code owner December 27, 2024 18:02
@lilyydu lilyydu changed the title DO NOT MERGE [JS] fix: streaming - citations and duplicate typing activity [JS] fix: streaming - citations and duplicate typing activity Dec 27, 2024
@lilyydu lilyydu changed the title [JS] fix: streaming - citations and duplicate typing activity [JS] fix: streaming - citations, feedback loop, and duplicate typing activity Dec 27, 2024
@corinagum corinagum merged commit b6b7386 into main Jan 6, 2025
12 of 14 checks passed
@corinagum corinagum deleted the lilyydu/stream-citations-js branch January 6, 2025 17:56
singhk97 pushed a commit that referenced this pull request Jan 6, 2025
@lilyydu
[C#] fix: streaming - citations and duplicate typing activity (#2202)
7f92562 
## Linked issues

closes: #minor

## Details
match with #2185, bug fixes & citations is now enabled during streaming,
and for the final message.


![image](https://github.com/user-attachments/assets/951c63e6-42cf-4280-9ef9-27efbd33fced)


## Attestation Checklist

- [x] My code follows the style guidelines of this project

- I have checked for/fixed spelling, linting, and other errors
- I have commented my code for clarity
- I have made corresponding changes to the documentation (updating the
doc strings in the code is sufficient)
- My changes generate no new warnings
- I have added tests that validates my changes, and provides sufficient
test coverage. I have tested with:
  - Local testing
  - E2E testing in Teams
- New and existing unit tests pass locally with my changes

---------

Co-authored-by: lilydu <lilydu+odspmdb@microsoft.com>
corinagum pushed a commit that referenced this pull request Jan 7, 2025
@lilyydu
[PY] fix: streaming - citations & pyproject.toml updates (#2205)
850c77b 
## Linked issues

closes: #minor

## Details

match with #2185 and #2202,  


![image](https://github.com/user-attachments/assets/06846624-0b4b-4913-9177-10016ec9b108)


- citations is now enabled during streaming, and for the final message
- updated colour/outline pngs for sample to adhere to manifest
validation
- fixed indexing for citations `position`
- added `StreamingEntity` class to fix serialization on `streaminfo`
object
- version 2 of poetry was released which led to a few scripts failing

## Attestation Checklist

- [x] My code follows the style guidelines of this project

- I have checked for/fixed spelling, linting, and other errors
- I have commented my code for clarity
- I have made corresponding changes to the documentation (updating the
doc strings in the code is sufficient)
- My changes generate no new warnings
- I have added tests that validates my changes, and provides sufficient
test coverage. I have tested with:
  - Local testing
  - E2E testing in Teams
- New and existing unit tests pass locally with my changes

---------

Co-authored-by: lilydu <lilydu+odspmdb@microsoft.com>
@corinagum corinagum mentioned this pull request Jan 8, 2025
Merged
corinagum added a commit that referenced this pull request Jan 8, 2025
@corinagum
[JS] chore: release JS 1.7.2 (#2258)
e61f929 
#minor

## Changes

* [JS] chore: Release 1.7.0 by @corinagum in
#2222
* [repo] bump: (deps): Bump github/codeql-action from 3.27.5 to 3.27.6
in the production group by @dependabot in
#2223
* [repo] docs: Update wording in the sample readmes for clarity by
@corinagum in #2225
* [repo] fix: C# Citations' `encodingFormat` to not be hardcoded. Fix JS
AssistantsPlanner options to set api version by @corinagum in
#2235
* [repo] bump: (deps): Bump github/codeql-action from 3.27.6 to 3.27.9
in the production group by @dependabot in
#2239
* [JS] bump: (deps-dev): Bump @microsoft/api-extractor from 7.48.0 to
7.48.1 in /js in the development group by @dependabot in
#2238
* [JS] Bump Teams AI for patch release 1.7.1 by @corinagum in
#2236
* [JS] fix: streaming - citations, feedback loop, and duplicate typing
activity by @lilyydu in #2185
* [JS] fix: Assistants Planner API version by @corinagum in
#2256
* [JS] bump: (deps-dev): Bump @types/lodash from 4.17.13 to 4.17.14 in
/js in the development group by @dependabot in
#2255
* [JS] bump: (deps): Bump the production group across 1 directory with 2
updates by @dependabot in
#2254
* [JS] bump: Updating Feedback buttons to the sample by @Jegadeesh-MSFT
in #2245

Co-authored-by: Corina Gum <>
This was referenced Jan 23, 2025
Closed
Merged
singhk97 added a commit that referenced this pull request Feb 3, 2025
@singhk97 @lilyydu
@corinagum @dependabot @ekzhu @Stevenic @jbaron @aacebo
[C#] feat: custom feedback form + citation changes (#2278)
30bc60d 
## Linked issues

closes: #2167 

## Details

ported over from: #2275, #2185 (feedback loop type streaming changes)

- Added `FeedbackLoopType` flag to configure the feedback loop type.
- Added `Application.OnMessageFetchTask` route registration method.

## Attestation Checklist

- [x] My code follows the style guidelines of this project

- I have checked for/fixed spelling, linting, and other errors
- I have commented my code for clarity
- I have made corresponding changes to the documentation (updating the
doc strings in the code is sufficient)
- My changes generate no new warnings
- I have added tests that validates my changes, and provides sufficient
test coverage. I have tested with:
  - Local testing
  - E2E testing in Teams
- New and existing unit tests pass locally with my changes

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Lily Du <lilydu@microsoft.com>
Co-authored-by: Corina <14900841+corinagum@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com>
Co-authored-by: Steven Ickman <stevenic@microsoft.com>
Co-authored-by: Peter <peter@neurallayer.com>
Co-authored-by: Alex Acebo <aacebowork@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corinagum corinagum corinagum approved these changes

@aacebo aacebo aacebo approved these changes

@singhk97 singhk97 Awaiting requested review from singhk97 singhk97 is a code owner

@rajan-chari rajan-chari Awaiting requested review from rajan-chari rajan-chari is a code owner

@heyitsaamir heyitsaamir Awaiting requested review from heyitsaamir heyitsaamir is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lilyydu @corinagum @aacebo