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

adds policy parameters to single experiment fetch #2197

Merged

Conversation

danoswaltCL
Copy link
Collaborator

@danoswaltCL danoswaltCL commented Feb 3, 2025

Adds the necessary call to get mooclet policy parameters and attach them to experiment when fetching single experiment by id in order to support showing this in the details page.

#2192

@zackcl
Copy link
Collaborator

zackcl commented Feb 5, 2025

I just tried creating a mooclet experiment on this branch, but I'm getting a 500 internal server error when making the /api/experiments POST request.

Here are the logs from the Terminal:

upgrade-app-1  | [1] TypeError: Cannot read properties of undefined (reading 'id')
upgrade-app-1  | [1]     at MoocletExperimentService.<anonymous> (/usr/src/app/backend/packages/Upgrade/src/api/services/MoocletExperimentService.ts:274:56)
upgrade-app-1  | [1]     at Generator.next (<anonymous>)
upgrade-app-1  | [1]     at fulfilled (/usr/src/app/backend/packages/Upgrade/node_modules/tslib/tslib.js:167:62)
upgrade-app-1  | [1]     at processTicksAndRejections (node:internal/process/task_queues:95:5)
postgres-1     | 2025-02-05 16:38:39.433 UTC [53] LOG:  checkpoint starting: time
postgres-1     | 2025-02-05 16:39:17.505 UTC [53] LOG:  checkpoint complete: wrote 367 buffers (2.2%); 0 WAL file(s) added, 0 removed, 0 recycled; write=38.018 s, sync=0.031 s, total=38.072 s; sync files=362, longest=0.002 s, average=0.001 s; distance=2696 kB, estimate=2696 kB; lsn=0/178CF18, redo lsn=0/178CE88

@zackcl zackcl self-requested a review February 5, 2025 16:41
@zackcl
Copy link
Collaborator

zackcl commented Feb 5, 2025

@danoswaltCL Strangely, I was able to create 2 mooclet experiments without errors after getting the above error. I will see if I can reproduce the error.

@zackcl
Copy link
Collaborator

zackcl commented Feb 5, 2025

@danoswaltCL It seems the error only happens when I name the experiment "Mooc Test". I believe I've used this experiment name previously before I cleared the local UpGrade DB (I didn't clear Mooclet DB though). Maybe the Mooclet server stored this experiment name and is complaining due to the duplicated name?

If the error is simply about the duplicated name, we can probably address this later in a separate PR.

@zackcl
Copy link
Collaborator

zackcl commented Feb 5, 2025

I'm not sure why the "Edit" button is hidden in the Mooclet experiment. I thought I changed this in #2198, but maybe I missed something. I will check this tomorrow.

Screenshot 2025-02-06 at 2 03 37 AM

@danoswaltCL
Copy link
Collaborator Author

Yeah, that's definitely a thing that needs to be addressed, it will give a bad error if the name already was used in Mooclet. I will create a separate ticket, we should have a validation check for duplicate name.

@zackcl
Copy link
Collaborator

zackcl commented Feb 6, 2025

@danoswaltCL I noticed that the changes made to the dev branch in PR #2198 don't seem to be applied to this branch. What's puzzling is that these differences don't show up in the diff (https://github.com/CarnegieLearningWeb/UpGrade/pull/2197/files).

For instance, your branch still contains the logic to hide the edit button for Mooclet experiments:
https://github.com/CarnegieLearningWeb/UpGrade/blob/feature/fetch-policyparam-details-experiment-view-backend/frontend/projects/upgrade/src/app/features/dashboard/home/pages/view-experiment/view-experiment.component.html#L33

This logic was removed from the dev branch in my PR:
https://github.com/CarnegieLearningWeb/UpGrade/blob/dev/frontend/projects/upgrade/src/app/features/dashboard/home/pages/view-experiment/view-experiment.component.html#L33

However, I can't see this change reflected in the "Files Changed" page:
https://github.com/CarnegieLearningWeb/UpGrade/pull/2197/files

How is this possible? Am I missing something here?

Copy link
Collaborator

@VivekFitkariwala VivekFitkariwala left a comment

Choose a reason for hiding this comment

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

Small fixes

@danoswaltCL
Copy link
Collaborator Author

danoswaltCL commented Feb 6, 2025

@zackcl i hadn't pulled changes from dev yet. I pulled that in and updated @VivekFitkariwala 's comments, ready for review.

@zackcl
Copy link
Collaborator

zackcl commented Feb 7, 2025

I think this looks good and works fine. Just wanted to mention that importing a Mooclet experiment is not working. This issue also exists in the dev branch. I'm not sure if it will be addressed in a different PR, though.

Screenshot 2025-02-07 at 6 23 34 PM

@danoswaltCL
Copy link
Collaborator Author

yes #2177 was created to handle imports, we talked about this in refinement and @bcb37 said he tried to get this error but couldn't reproduce. I added your screenshot to the story and wrote more accurate requirements.

I think this looks good and works fine. Just wanted to mention that importing a Mooclet experiment is not working. This issue also exists in the dev branch. I'm not sure if it will be addressed in a different PR, though.

Screenshot 2025-02-07 at 6 23 34 PM

const experiment = await this.experimentService.getSingleExperiment(id, request.logger);

if (SUPPORTED_MOOCLET_ALGORITHMS.includes(experiment.assignmentAlgorithm)) {
if (!env.mooclets?.enabled) {
Copy link
Collaborator

@VivekFitkariwala VivekFitkariwala Feb 9, 2025

Choose a reason for hiding this comment

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

This if check should be a util function which should throw an error if the experiment contains a valid mooclet algorithm and mooclet is not enabled, otherwise return true.
It is getting in the create function as well and even Ben is using this in his PR

Might be in a separate PR or refactoring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure that makes sense, let me make a separate ticket for that for follow-up

@danoswaltCL danoswaltCL merged commit 2eb265d into dev Feb 10, 2025
14 of 15 checks passed
@danoswaltCL danoswaltCL deleted the feature/fetch-policyparam-details-experiment-view-backend branch February 10, 2025 15:01
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