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

Check task overwriting through ProFormA #1320

Open
9 of 18 tasks
MrSerth opened this issue Mar 25, 2024 · 9 comments · May be fixed by #1696
Open
9 of 18 tasks

Check task overwriting through ProFormA #1320

MrSerth opened this issue Mar 25, 2024 · 9 comments · May be fixed by #1696

Comments

@MrSerth
Copy link
Member

MrSerth commented Mar 25, 2024

When pushing an existing task through our ProFormA API, one gets ask whether the existing task should be overwritten (given sufficient permissions) or whether a new copy should be created. That's fine and working as expected.

My questions:

  • Is overwriting the task keeping or resetting further data previously set in CodeHarbor that is not sent through ProFormA? For example, I am referring to
    • labels
    • groups
    • the license selected
    • collections
    • comments
    • ratings
    • the visibility level
    • the parent UUID
  • How is CodeOcean behaving there with regards to the CodeOcean-specific data:
    • tips
    • tags
    • LTI parameters
    • collections
    • proxy exercises
    • learner feedback
    • learner submissions, test runs
    • RfCs

I was unsure (haven't tested it myself) and would like to know how the systems behave. If any of these data gets lost, I would strongly suggest to keep this "additional" data, only overwriting those "attributes" sent through the API.

@kkoehn
Copy link
Collaborator

kkoehn commented May 13, 2024

In CodeHarbor we work with assign_attributes

      @task.assign_attributes(
        user: @user,
        title: @proforma_task.title,
        description:,
        internal_description: @proforma_task.internal_description,
        programming_language:,
        uuid:,
        parent_uuid: @proforma_task.parent_uuid,
        language: @proforma_task.language,
        meta_data: @proforma_task.meta_data,

        submission_restrictions: @proforma_task.submission_restrictions,
        external_resources: @proforma_task.external_resources,
        grading_hints: @proforma_task.grading_hints,

        tests:,
        model_solutions:,
        files: files.values # this line has to be last, because tests and model_solutions have to remove their respective files first
      )

Therefore all mentioned relations stay in place and only attributes get changed. parent_uuid will get overwritten, because it's part of proforma. access_level is not touched and will also stay the same. For some reason user is overwritten - I would suggest removing that part, because I don't see a reason to change the ownership of a task because of an import.

@kkoehn
Copy link
Collaborator

kkoehn commented May 13, 2024

In CodeOcean we do it similarly:

      @exercise.assign_attributes(
        user: @user,
        title: @task.title,
        description: @task.description,
        public: string_to_bool(extract_meta_data(@task.meta_data&.dig('meta-data'), 'public')) || false,
        hide_file_tree: string_to_bool(extract_meta_data(@task.meta_data&.dig('meta-data'), 'hide_file_tree')) || false,
        allow_file_creation: string_to_bool(extract_meta_data(@task.meta_data&.dig('meta-data'), 'allow_file_creation')) || false,
        allow_auto_completion: string_to_bool(extract_meta_data(@task.meta_data&.dig('meta-data'), 'allow_auto_completion')) || false,
        expected_difficulty: extract_meta_data(@task.meta_data&.dig('meta-data'), 'expected_difficulty') || 1,
        execution_environment_id:,

        files:
      )

Here me extract potentially saved meta-data in our own cool format. We reassign user just like in CH.
We do, however, replace existing files by creating new ones and deleting the old ones. If any data is linked to the files, we might have to take a look at that.

@MrSerth
Copy link
Member Author

MrSerth commented May 20, 2024

Thanks for checking! That's a valuable finding and seems to be mostly okay; we probably have to change only a few minor things:

CodeHarbor:

  • Overwriting the parent_uuid in general is fine. However, in conjunction with CodeOcean that's problematic since CodeOcean is not aware of any parent_uuid. Imagine I export a CodeHarbor task to CodeOcean which had a parent_uuid set. After some changes in CodeOcean, I re-export the very same task back to CodeHarbor, which would result in loosing the parent_uuid. This should be fixed, so that the parent_uuid is not lost (maybe by exporting it to CodeOcean as well?)
  • Overwriting user is odd, and should be removed. We won't update the user when updating the task through the CodeHarbor UI.
  • Do I get right that we are also re-creating tests, model_solutions, and files in CodeHarbor completely (and dropping the previous objects)? This might change overwrite the created_at value and:
    • Test#validity
    • Test#timeout
    • Test#testing_framework
    • ... is probably an issue with the new parent_id introduced in TaskContributions #1103 for three tests, model_solutions, and files, since these IDs might need an update, too: For a valid task, none of these values is ever set (only for a contribution), but the ID saved in other objects could refer to one of the artifacts being updated through a ProFormA import.

CodeOcean:

  • Reassigning the user is not too problematic, since this mirrors the behavior for updates through the UI (where we also change the user). Not ideal, but should be addressed together, I'd say.
  • Similarly to CodeHarbor, we have an issue with recreating the files: The file ID of an exercise is used several times. For one, it is used as a target in the CodeOcean::Files for the column file_id when the context_type is set to Submission (same design principle as with the parent_id in CodeHarbor). Further, it is also used in Testruns for column file_id. Potentially, RequestForComments (and LinterCheckRun) is also affected.
  • Otherwise, I am not aware of other data being linked to files (they are rather linked to the exercise, which is fine).

That having said, I am afraid we probably need to change some of the behavior (mainly to keep and update the existing objects).

@MrSerth
Copy link
Member Author

MrSerth commented Aug 4, 2024

Note to self: As soon as overwriting is possible without negative consequences, we should re-export all former Python 3.7 exercises from CodeOcean to CodeHarbor (they have been updated to use the Python 3.8 environment now).

@MrSerth
Copy link
Member Author

MrSerth commented Sep 3, 2024

I've tested the behavior with #1612 and updated the original checklist based on this PR. With these changes, re-exporting a task to CodeHarbor is fine.

However, we still need to tackle the problems in CodeOcean before closing this issue.

@MrSerth
Copy link
Member Author

MrSerth commented Sep 3, 2024

I've updated the tasks on CodeHarbor, so that I don't forget doing so later. During this update, I noticed a remaining bug in #1612. As part of that, the update process falsely recreated some TaskFiles (changing their ID) which is not ideal but happened before introducing contributions.

@MrSerth
Copy link
Member Author

MrSerth commented Sep 25, 2024

While reviewing openHPI/codeocean#2538 (review), I noticed we still have an issue even after merging #1612. That should be investigated before closing this issue (see the attached comment for details).

@kkoehn
Copy link
Collaborator

kkoehn commented Oct 2, 2024

Regarding the issue in CodeHarbor, I traced the problem to a file that was originally assigned to a test, which no longer exists, and is now being used as a task_file.

This situation causes the deletion of the test (and, consequently, the file) before upsert_files @proforma_task, @task is called. While the file is still found, the destroyed? flag is already true, which means the file's attributes are frozen.

A strange issue occurs when moving upsert_files before assign_attributes: the file gets correctly assigned to the task, and the test no longer has any files. However, when the test is removed, the file is still being deleted. I assume that #destroy is being called by assign_attributes, which does not account for intermediate changes, and instead operates directly on the database state.

I created a PR with my experiments and tests, but I was not able to find a working solution.

@kkoehn kkoehn linked a pull request Oct 2, 2024 that will close this issue
@kkoehn kkoehn linked a pull request Oct 2, 2024 that will close this issue
@MrSerth
Copy link
Member Author

MrSerth commented Oct 9, 2024

Thank you for this work. I was able to reproduce the issue and didn't find an optimal solution either. Nevertheless, I've made a proposal that should resolve the issue, see #1696 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants