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

Fixes part of #18442: Remove deprecated edit state content suggestion model #20079

Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
fbfde4b
wrote and regidtered beam job
masterboy376 Mar 29, 2024
ee7ed03
fic failing checks
masterboy376 Mar 29, 2024
451ee1e
fixed some linting issues
masterboy376 Mar 29, 2024
a82ee41
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Mar 29, 2024
c509608
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Mar 30, 2024
59659a5
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Mar 31, 2024
cf130cb
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Apr 1, 2024
e2e9297
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Apr 1, 2024
4335287
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Apr 3, 2024
d6b6850
address reviewer comments
masterboy376 Apr 3, 2024
20fb747
modified test shards
masterboy376 Apr 6, 2024
926dc2b
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Apr 6, 2024
fdaf85a
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Apr 8, 2024
d80039d
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Apr 8, 2024
ab4829f
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Apr 10, 2024
d52ea67
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Apr 11, 2024
5ff1b63
addressed reviewer comments.
masterboy376 Apr 11, 2024
bb11d07
addressed reviewer comments
masterboy376 Apr 11, 2024
91b9826
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Apr 12, 2024
c7c63ea
addressed reviewers comment
masterboy376 Apr 13, 2024
cbbd796
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Apr 15, 2024
ccf71de
addressed reviewer comments
masterboy376 Apr 15, 2024
6e44674
addressed reviewer comments
masterboy376 Apr 15, 2024
a7ae87f
Merge remote-tracking branch 'upstream/develop' into remove_deprecate…
masterboy376 Apr 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 123 additions & 0 deletions core/jobs/batch_jobs/suggestion_edit_state_content_deletion_jobs.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
# coding: utf-8
#
# Copyright 2024 The Oppia Authors. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS-IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

"""Deletion Jobs for edit state content suggestion models"""
Copy link
Member

Choose a reason for hiding this comment

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

Deletion jobs ...

End the docstring with a period.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done


from __future__ import annotations

from core import feconf
from core.jobs import base_jobs
from core.jobs.io import ndb_io
from core.jobs.transforms import job_result_transforms
from core.jobs.types import job_run_result
from core.platform import models

import apache_beam as beam
from typing import Tuple

MYPY = False
if MYPY: # pragma: no cover
from mypy_imports import suggestion_models

(suggestion_models, ) = models.Registry.import_models([
models.Names.SUGGESTION])


# TODO(#15613): Here we use MyPy ignore because the incomplete typing of
# apache_beam library and absences of stubs in Typeshed, forces MyPy to
# assume that PTransform class is of type Any. Thus to avoid MyPy's error
# (Class cannot subclass 'PTransform' (has type 'Any')), we added an
# ignore here.
class GetDeprecatedSuggestionEditStateContentModels(beam.PTransform): # type: ignore[misc]
"""Transform that gets all edit state content suggestion models."""

def expand(
self, pipeline: beam.Pipeline
) -> Tuple[
beam.PCollection[suggestion_models.GeneralSuggestionModel],
beam.PCollection[job_run_result.JobRunResult]
]:
suggestion_edit_state_content_model_to_delete = (
pipeline
| 'Get all general suggestion models' >> ndb_io.GetModels(
suggestion_models.GeneralSuggestionModel.get_all())
| 'Filter edit state content suggestion' >> (
beam.Filter(
lambda model: ((
model.suggestion_type == (
feconf.SUGGESTION_TYPE_EDIT_STATE_CONTENT)) and (
model.target_type == (
Copy link
Member

Choose a reason for hiding this comment

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

Do you actually need this check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here, first we are fetching all the general suggestion models the filtering needed models using model.suggestion_type == feconf.SUGGESTION_TYPE_EDIT_STATE_CONTENT. However, here we can remove later filter condition that is model.target_type == feconf.ENTITY_TYPE_EXPLORATION, if all SUGGESTION_TYPE_EDIT_STATE_CONTENT models are of target type ENTITY_TYPE_EXPLORATION, and this seems true after the list of SUGGESTION_TYPE_EDIT_STATE_CONTENT models that you provided earlier.

feconf.ENTITY_TYPE_EXPLORATION)))
))
)

suggestion_edit_state_content_model_to_delete_count = (
suggestion_edit_state_content_model_to_delete
| 'Count edit state content suggestion to be deleted' >> (
job_result_transforms.CountObjectsToJobRunResult(
'EDIT STATE CONTENT SUGGESTION'))
)

return (
suggestion_edit_state_content_model_to_delete,
Copy link
Member

Choose a reason for hiding this comment

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

Deindent this and the next line by 4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

suggestion_edit_state_content_model_to_delete_count,
)


class DeleteDeprecatedSuggestionEditStateContentModelsJob(base_jobs.JobBase):
"""Job that deletes edit state content suggestion models as these are
deprecated.
"""

def run(self) -> beam.PCollection[job_run_result.JobRunResult]:

(suggestion_edit_state_content_model_to_delete, (
suggestion_edit_state_content_model_to_delete_result)) = (
self.pipeline
| 'Get edit state content suggestion models' >> (
GetDeprecatedSuggestionEditStateContentModels())
)

unused_models_deletion = (
Copy link
Member

Choose a reason for hiding this comment

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

unused_models_deletion_result

Though, if it's unused, then why bother to declare it in the first place?

Copy link
Collaborator Author

@masterboy376 masterboy376 Apr 13, 2024

Choose a reason for hiding this comment

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

We need to assign expression "suggestion_edit_state_content_model_to_delete | 'Extract keys' >> beam.Map(lambda model: model.key) | 'Delete models' >> ndb_io.DeleteModels()" to something to avoid linting error reading "expression is assigned to nothing", so, I assigned it to unused_models_deletion.

Copy link
Member

Choose a reason for hiding this comment

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

OK, can you rename to unused_models_deletion_result then (as stated in the earlier comment)?

(
suggestion_edit_state_content_model_to_delete
)
| 'Extract keys' >> beam.Map(lambda model: model.key)
| 'Delete models' >> ndb_io.DeleteModels()
)

return (
(
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this parens and the indentation below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

suggestion_edit_state_content_model_to_delete_result,
)
| 'Merge results' >> beam.Flatten()
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering, why do we not report these the same way as in the audit job?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Totally agree, we actually don't need to use beam.Flatten() here.
Made required changes.

)


class AuditDeprecatedSuggestionEditStateContentModelsDeletionJob(
base_jobs.JobBase):
"""Job that audit edit state content suggestion."""

def run(self) -> beam.PCollection[job_run_result.JobRunResult]:

job_run_results = (
self.pipeline
| 'Perform fetching and deletion of edit state content' +
' suggestion results' >> (
GetDeprecatedSuggestionEditStateContentModels())
)[1]

return job_run_results
Loading
Loading