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

Prompt for reason for skipping. #520

Merged
merged 1 commit into from
Jan 8, 2023

Conversation

othrayte
Copy link
Collaborator

@othrayte othrayte commented Jan 8, 2023

This is a draft for #310 (Add ability to tag tasks as unanswerable).

It tasks onboard the feedback that we want to guide people away from skipping a bit and that we want to know why they have skipped. The leading idea for that appeared to be allowing the user to provide a custom reason, so that is the direction this went.

The main reason that this is only a draft PR is that at the moment we are not able to NACK after ACK-ing and the website auto-ACKs on fetching tasks so there is no chance to NACK. In this PR I have commented out the auto-ACK-ing to allow for testing this.

@@ -37,7 +37,8 @@ const handler = async (req, res) => {
});

// Update the backend with our Task ID
await oasstApiClient.ackTask(task.id, registeredTask.id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Controversial proposal: What if we really delete this here and move the auto-ack to when we submit the answer?

Copy link
Collaborator

@AbdBarho AbdBarho Jan 8, 2023

Choose a reason for hiding this comment

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

in that case, shouldn't the auto ack happen in the (real) backend when we submit?
I don't see the advantage of acking & submitting immediately.

TBH, I still don't fully understand why we ack in the first place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree the acking is a bit weird. Let's discuss in the next call about how needed it really is. For now let's pretend its required and just work around the API as is.

import { oasstApiClient } from "src/lib/oasst_api_client";

/**
* Returns a new task created from the Task Backend. We do a few things here:
Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll want to delete these comments

Copy link
Collaborator

@fozziethebeat fozziethebeat left a comment

Choose a reason for hiding this comment

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

Overall I think this makes sense if we move the auto-acking.

Note: skipping is only half implemented for labelling and summarisation tasks as those probably need to be changed to use the new Task component.
@othrayte
Copy link
Collaborator Author

othrayte commented Jan 8, 2023

I've moved the ACK to just before submitting interactions. Also I've fixed the summerisation and labeling tasks temporarily by making them skip without NACK-ing as they don't yet use the new Task.tsx component and I wasn't sure if someone was already working on doing that and figured that would be the better way to handle them.

@othrayte othrayte marked this pull request as ready for review January 8, 2023 13:10
@othrayte othrayte requested review from AbdBarho and fozziethebeat and removed request for AbdBarho and fozziethebeat January 8, 2023 13:11
@othrayte
Copy link
Collaborator Author

othrayte commented Jan 8, 2023

Apologies for any review request spam, I'm still learning how that action works on github.

@fozziethebeat
Copy link
Collaborator

Looks good, we can fix any small issues later.

@fozziethebeat fozziethebeat merged commit ff4e1c7 into LAION-AI:main Jan 8, 2023
@othrayte othrayte deleted the skip-with-reason branch January 9, 2023 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants