[change] estimate_release - make configurable #3650

aidan- · 2023-01-03T01:12:38Z

Motivation for changes:

Primarily driven by the feature request in #3614.

Detailed changes:

Before this PR, the estimate_release plugin, used by other plugins like the discover plugin, was not user configurable. Some config in the discover plugin allowed users to config modify the estimated release, but the functionality was limited to specific modes of operation.

Rather than complicating the discover plugin with logic to control what should be the responsibility of the estimate_release plugin, this PR extends the estimate_release plugin to allow some basic configuration - such as modifying the calculated estimated release date and restricting which estimator providers to use.

This PR also removes the optimistic mode from discover as that functionality is now supported through the configuration of estimate_release across all modes.

Addressed issues/feature requests:

Fixes Feature request: a combination of loose and optimistic for release estimations #3614

Config usage if relevant (new plugin or updated schema):

        tasks:
          test_estimates:
            discover:
              interval: 0 seconds
              what:
              - mock:
                - title: Foo
              from:
              - test_search: yes
            estimate_release:
              offset: -7 days
              providers: ["movies_bluray"]

To Do:

Update wiki
Further integration testing if the approach is on track

paranoidi · 2023-01-04T16:32:13Z

I'm a bit torn how this adds the release estimation configuration to root level of the plugin. Although it's definitely not a deal breaker here.

I might prefer if it was value for each mode instead, like with optimistic option was but being optional.

Good work though so far :)

Any thoughts from other coders and users?

gazpachoking

Yeah, I think this is an improvement from what we have.

It made me realize though, that we have a modular system for estimating releases, but at the moment the user has no control over said system. I wonder if we should add some contol over that similar to how we did for the parsing system. Such that a task could look something like:

discover:
  what: blah
  from: blah
release_estimation:
  providers: [bluray, tvmaze]
  adjustment: -7 days

That would probably be a task for a separate PR though.

flexget/plugins/input/discover.py

paranoidi · 2023-01-04T17:20:51Z

Yeah, I think this is an improvement from what we have.

It made me realize though, that we have a modular system for estimating releases, but at the moment the user has no control over said system. I wonder if we should add some contol over that similar to how we did for the parsing system. Such that a task could look something like:
discover:
  what: blah
  from: blah
release_estimation:
  providers: [bluray, tvmaze]
  adjustment: -7 days
That would probably be a task for a separate PR though.

That makes much more sense and I like the configuration format much more as it moves all estimation related configuration under release_estimations key. Better for context, better name (adjustment). If we want to go with that route though, then this PR should probably not be merged at all.

@aidan- Would you be up for implementing what gazpachoking outlined?

Edit: adding support for listing providers being optional / another PR, just change the release_date_modifier to be compatible with the plan above.

flexget/plugins/input/discover.py

aidan- · 2023-01-05T08:39:22Z

I might prefer if it was value for each mode instead, like with optimistic option was but being optional.

That can work, but to me the existing optimistic: <interval> structure looks pretty illogical and ambiguous - the mix of config structure just for this operating mode makes that even more so. Modifying the release date is also something that should be configured across all of the modes (except ignore obviously) which to me says it probably isn't a property of the estimation mode but of the discover itself (or the release_estimation as @gazpachoking suggested!).

That makes much more sense and I like the configuration format much more as it moves all estimation related configuration under release_estimations key. Better for context, better name (adjustment). If we want to go with that route though, then this PR should probably not be merged at all.
@aidan- Would you be up for implementing what gazpachoking outlined?

Yeah for sure! I think that would look much cleaner and more logical.

paranoidi · 2023-01-05T20:21:49Z

Yeah for sure! I think that would look much cleaner and more logical.

Awesome, I think we all are in agreement. Waiting for PR updates when you have the time :)

paranoidi · 2023-01-05T22:17:46Z

Another possible name for the option would be "offset", which I kinda like even more.

BrutuZ · 2023-01-05T22:38:14Z

Another possible name for the option would be "offset", which I kinda like even more.

Maybe estimator_offset to keep what it is offsetting clear, otherwise it could be thought to be affecting the interval setting of discover

aidan- · 2023-01-26T01:26:57Z

@paranoidi bit slow on this one, real life got in the way. I still need to do some integration testing to make sure its all functional but figured I'd get some eyes on it now to make sure its in the right direction.

EDIT: Reading through the comments again, the 'mode' that controls how the release date result is handled is still apart of the discover plugin. Happy to pull that out into the estimate_release plugin in a separate PR if there is value.

paranoidi · 2023-01-26T18:25:03Z

flexget/components/estimate_release/estimate_release.py

+        self.task_estimate_config = {}
+
+    @property
+    def schema(self) -> Dict[str, Any]:


This is not the way to handle sub plugins & interfaces, please look at how discover plugin handles search interface.

Sure. I'll take a look.

paranoidi · 2023-01-26T18:43:31Z

flexget/components/estimate_release/estimate_release.py

+    def on_task_start(self, task: Task, config) -> None:
+        # Load task specific estimator configuration
+        if config:
+            self.task_estimate_config = config
+
+    def on_task_exit(self, task: Task, config) -> None:
+        # Restore default estimator configuration for next task run
+        self.task_estimate_config = {}


This modifying configuration and restoring later is not a good idea. Generally speaking plugins should be stateless and not rely on __init__.

Interesting, this is similar to how the parsing plugin does it which was mentioned in one of the above comments as a reference.

Hmm, apologies. I guess there's no better way for it in these kinds of situations then. There is still issue with having self.task_estimate_config and the one declared in module level (L18). I don't think the module one is used and can be removed.

Yeah, we have to do some hacky stuff for bits that don't directly get a task parameter, there isn't a better way at the moment. I was thinking of making a plugin method that worked like a context manager, to make plugins that do setup and teardown around tasks a bit easier. Something like:

def on_task_context(self, task, config): if config: self.task_estimate_config = config try: yield finally: self.task_estimate_config = {}

That way you don't have to hook task_start, task_exit, and task_abort trying to manage state. @paranoidi Thoughts?

I also had this crazier idea based on that, but my gut tells me it's probably bad. It would allow a plugin to retain state during a task without storing things to self.

def on_task_phases(self, task, config): yield 'start' setupstuff = somefunc() yield 'filter' for entry in task.all_entries: do_something(entry, setupstuff) yield 'exit' teardown(setupstuff)

Heh, sure. I have some thoughts. Not overly sold on the idea.

I've been thinking years that we should have a baseclass for plugins and in there have a prepare_config method that plugins can override to format config with default values etc. Then passing the already prepared config into the usual phase methods. Also this would make it possible to declare the phase methods in the baseclass making them much more explicit. Last time I checked there was only one dynamically added phase which would not be a hard to get rid of.

I've been thinking years that we should have a baseclass for plugins and in there have a prepare_config method that plugins can override to format config with default values etc. Then passing the already prepared config into the usual phase methods.

Yeah, I love this idea.
I think it's totally separate than a context manager type thing to do cleanup, and I'm not totally sold on that idea either. The problem of doing setup and teardown in on_task_start/exit is that you can't guarantee that they run in pairs. I just fixed an issue with the parsing plugin where the on_task_abort cleanup ran even though the task was aborted before its on_task_start even set up the thing it was trying to clean up. You could also run into the issue of an abort happening after on_task_exit handler was run, and it tries to clean up twice. Here is a naive search that shows some plugins that might be able to use a thing like the context manager hook.

Maybe the baseclass could introduce setup/teardown methods (although I kinda dislike those names as they are heavily unit test related) that are guaranteed to run as expected. It would be much more explicit than relying on on_task_start etc.

github-actions · 2023-06-29T02:10:31Z

This PR is stale because it has been open 150 days with no activity. Remove stale label or comment or this will be closed in 60 days.

github-actions · 2024-01-21T01:43:23Z

This PR is stale because it has been open 150 days with no activity. Remove stale label or comment or this will be closed in 60 days.

aidan- mentioned this pull request Jan 3, 2023

Feature request: a combination of loose and optimistic for release estimations #3614

Closed

paranoidi requested a review from gazpachoking January 4, 2023 16:32

paranoidi added the Enhancement label Jan 4, 2023

gazpachoking reviewed Jan 4, 2023

View reviewed changes

flexget/plugins/input/discover.py Outdated Show resolved Hide resolved

flexget/plugins/input/discover.py Outdated Show resolved Hide resolved

BrutuZ reviewed Jan 4, 2023

View reviewed changes

flexget/plugins/input/discover.py Outdated Show resolved Hide resolved

aidan- added 3 commits January 8, 2023 20:46

fix isort

848cde1

refactor so estimate release is configurable

118d249

typo

163d531

aidan- changed the title ~~[change] discover - support release date modifier in all modes~~ [change] estimate_release - make configurable Jan 26, 2023

aidan- force-pushed the discover-clean-up-date-modifier branch from 71a2020 to 163d531 Compare January 26, 2023 01:24

paranoidi requested changes Jan 26, 2023

View reviewed changes

github-actions bot added the Stale label Jun 29, 2023

gazpachoking removed the Stale label Jun 29, 2023

github-actions bot added the Stale label Jan 21, 2024

gazpachoking removed the Stale label Jan 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[change] estimate_release - make configurable #3650

[change] estimate_release - make configurable #3650

aidan- commented Jan 3, 2023 •

edited

paranoidi commented Jan 4, 2023

gazpachoking left a comment

paranoidi commented Jan 4, 2023 •

edited

aidan- commented Jan 5, 2023

paranoidi commented Jan 5, 2023

paranoidi commented Jan 5, 2023

BrutuZ commented Jan 5, 2023

aidan- commented Jan 26, 2023 •

edited

paranoidi Jan 26, 2023

aidan- Jan 26, 2023

paranoidi Jan 26, 2023 •

edited

aidan- Jan 26, 2023 •

edited

paranoidi Jan 29, 2023

gazpachoking Aug 15, 2023

paranoidi Aug 16, 2023

gazpachoking Aug 16, 2023

paranoidi Aug 23, 2023

github-actions bot commented Jun 29, 2023

github-actions bot commented Jan 21, 2024

[change] estimate_release - make configurable #3650

Are you sure you want to change the base?

[change] estimate_release - make configurable #3650

Conversation

aidan- commented Jan 3, 2023 • edited

Motivation for changes:

Detailed changes:

Addressed issues/feature requests:

Config usage if relevant (new plugin or updated schema):

To Do:

paranoidi commented Jan 4, 2023

gazpachoking left a comment

Choose a reason for hiding this comment

paranoidi commented Jan 4, 2023 • edited

aidan- commented Jan 5, 2023

paranoidi commented Jan 5, 2023

paranoidi commented Jan 5, 2023

BrutuZ commented Jan 5, 2023

aidan- commented Jan 26, 2023 • edited

paranoidi Jan 26, 2023

Choose a reason for hiding this comment

aidan- Jan 26, 2023

Choose a reason for hiding this comment

paranoidi Jan 26, 2023 • edited

Choose a reason for hiding this comment

aidan- Jan 26, 2023 • edited

Choose a reason for hiding this comment

paranoidi Jan 29, 2023

Choose a reason for hiding this comment

gazpachoking Aug 15, 2023

Choose a reason for hiding this comment

paranoidi Aug 16, 2023

Choose a reason for hiding this comment

gazpachoking Aug 16, 2023

Choose a reason for hiding this comment

paranoidi Aug 23, 2023

Choose a reason for hiding this comment

github-actions bot commented Jun 29, 2023

github-actions bot commented Jan 21, 2024

aidan- commented Jan 3, 2023 •

edited

paranoidi commented Jan 4, 2023 •

edited

aidan- commented Jan 26, 2023 •

edited

paranoidi Jan 26, 2023 •

edited

aidan- Jan 26, 2023 •

edited