Class Tune #1200

GooseIt · 2023-04-02T10:49:10Z

Before submitting (must do checklist)

Did you read the contribution guide?
Did you update the docs? We use Numpy format for all the methods and classes.
Did you write any new necessary tests?
Did you update the CHANGELOG?

Proposed Changes

See #1024

Closing issues

closes #1024

…class_tune

GooseIt · 2023-04-02T10:51:06Z

Here is a list of details of my implementation that I think should be stated:

Typing of BaseMixin.set_params() was changed. Now the method returns type of object, from which it was called (instead of always returning BaseMixin). In Python 3.11 this can be implemeted easily using Typing.Self. So when Etna updates Python version it operates on, I think this implementation should be changed as well.
The implementation of BaseMixin.set_params() helper-function BaseMixin._update_nested_dict_with_flat_dict() altered to make it able to process .params_to_tune() output (original implementation was built to operate only with .to_dict() output). It is made possible by allowing list parsing during param_nesting-processing cycle, this option is currently only used when indexing "i"th element of transforms list during handling of "transforms.i.x" input.
(!!!) Current implementation of Tune is base on assumption that params_to_tune yields transforms in format "transforms.i.x" instead of "transform.i.x". This decision of mine also affects test_can_handle_transforms in test/test_auto/test_tune.py. There are my reasons:
- "transforms" is the name by which list of transforms is passed when constructing object from .to_dict() method output. I think there should be consistency around the code in dict elements naming, having it "transforms" in one place and "transform" in others could confuse contributors.
- "transforms.i.x" notation is interpretable - it is understandable that there are multiple transforms, and i refers to number of concrete transform. On the other hand, it is unclear whether i in "transform.i.x" refers to indice of transform or to indice of something inside one transform object.
- implementation-wise i think it is better to have the same processing pipeline for transforms coming from .to_dict() and from .params_to_tune(). I don't think that checking if coming arguments are "transform" or "transforms" and then directing them into one of two distinct pipelines is a good idea.

If you think that my decision (or any of the arguments) to prefer "transforms.i.x" notation is wrong, please explain why. I'm ready to re-write my code if you provide your vision on implementation of Tune based on keeping "transform.i.x" notation.

codecov-commenter · 2023-04-02T10:53:14Z

Codecov Report

❗ No coverage uploaded for pull request base (automl-2.0@3c42bad). Click here to learn what that means.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@              Coverage Diff              @@
##             automl-2.0    #1200   +/-   ##
=============================================
  Coverage              ?   88.34%           
=============================================
  Files                 ?      186           
  Lines                 ?    11132           
  Branches              ?        0           
=============================================
  Hits                  ?     9835           
  Misses                ?     1297           
  Partials              ?        0

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

GooseIt · 2023-04-02T10:53:23Z

There is also 1 problem:
As you can see, I've declared dict_of_distrs right inside objective function.
I don't think that's good, as it creates dict_of_distrs every time objective is called, yet I don't know a better place to put dict_of_distrs declaration.
Please provide with suggestion where it should be put.

Mr-Geekman · 2023-04-03T11:36:37Z

Ok, it is also possible to import Self from typing_extensions, but our current mypy version doesn't support it, as I understand.
I fixed this in this PR: Teach set_params to work with nested list and tuple #1201.
I agree about transforms, but don't really see where it was said to use transform.

etna/auto/auto.py

etna/core/mixins.py

Mr-Geekman · 2023-04-03T11:41:54Z

CHANGELOG.md

@@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## Unreleased
 ### Added
+- Forecast decomposition for `SeasonalMovingAverageModel`([#1180](https://github.com/tinkoff-ai/etna/pull/1180))


I'll update automl-2.0 branch to hide this updates.

Do I have to remove it, or will you alter it during merge into automl-2.0?

I think it will be better to remove changes not related to the task from this branch.

Changelog shouldn't be like this. In actual changelog for automl-2.0 we have already added 2.0 release section. You should fix this.
May be the easiest way is to copy actual changelog and add only your changes.

Same for any other change for other classes that is unrelated to the task.

etna/auto/auto.py

Mr-Geekman · 2023-04-03T12:17:33Z

Some tests are also failing. Probably, they can be debugged locally.

Mr-Geekman · 2023-04-06T07:16:31Z

Hi, @GooseIt! Are there any news about fixes of the PR?

GooseIt · 2023-04-06T18:43:20Z

I'll take my time fixing this PR on this weekend, and I'll report no later than Sunday 23:59.

tests/test_auto/test_tune.py

GooseIt · 2023-04-09T21:42:01Z

I'm sorry, I've overesrimated my coding pacing. I'm looking into the issue, but it will take one or two more days to fix everything

GooseIt · 2023-04-11T18:22:09Z

I get the message below when running pytest tests/test_auto/test_auto.py inside poetry shell.
E RuntimeError: The runtime optuna version 2.10.1 is no longer compatible with the table schema (set up by optuna 3.1.0). Please try updating optuna to the latest version by $ pip install -U optuna.
The problem is, it is not exclusive to the branch I'm currently working in. I've checkout into master, and I there get the same error when running above mentioned command.
Do you get this error on your end as well?
If not, what do you think is the cause? Is there any problem with my virtual environment on your mind?

Mr-Geekman

I think logical part of the code looks good. Remaining tasks

Remove changes unrelated to the task.
Fix comments on tests.
Remove redundant 'print's.

Mr-Geekman · 2023-04-12T07:07:42Z

CHANGELOG.md

@@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## Unreleased
 ### Added
+- Forecast decomposition for `SeasonalMovingAverageModel`([#1180](https://github.com/tinkoff-ai/etna/pull/1180))


I think it will be better to remove changes not related to the task from this branch.

etna/auto/auto.py

Mr-Geekman

Sorry, I missed the button.

GooseIt · 2023-04-12T13:41:17Z

@Mr-Geekman, could you please reply to my E RuntimeError: The runtime optuna version 2.10.1 is no longer compatible with the table schema (set up by optuna 3.1.0). Please try updating optuna to the latest version by $ pip install -U optuna. error message?

This manifestation makes it hard for me to debug tests locally, as all tests dependent on optuna.storages.RDBStorage throw errors

Mr-Geekman · 2023-04-12T14:05:20Z

Have you seen somewhere in the tests the file with a name test.db? I have a suspicion that error is due to this file schema.

GooseIt · 2023-04-12T20:27:18Z

Yes, I believe that the file is relevant, all the tests fail on yield RDBStorage("sqlite:///test.db") from optuna_storage fixture

Mr-Geekman · 2023-04-12T21:45:16Z

Have you tried to remove it? Or it isn't present on the file system?

Mr-Geekman · 2023-04-12T21:46:25Z

I think this file is created somewhere in test_auto.py or somewhere around the fixture that creates it.

Mr-Geekman · 2023-04-16T11:22:10Z

@GooseIt , how are you doing with the task?

GooseIt · 2023-04-20T11:52:59Z

@Mr-Geekman, can this task wait for two weeks?
I think that this time will be more than enough for me to resolve my current schedule and have nothing else on my hands.

Mr-Geekman · 2023-04-25T15:58:53Z

We moved the task into the Hold status. Please, write here in the comments when you will be ready to continue.

…class_tune

GooseIt · 2023-05-15T16:31:32Z

It seems for me like everything except CHANGELOG.md is fixed.
Please review

Mr-Geekman

We should remove changes, unrelated to the task. It can even be done manually by copy-pasting.

There are some issues with tests.

If you are to tired of this task we can finish on removing changes from other code and I'll continue from there.

etna/auto/auto.py

Mr-Geekman · 2023-05-16T08:20:48Z

CHANGELOG.md

@@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## Unreleased
 ### Added
+- Forecast decomposition for `SeasonalMovingAverageModel`([#1180](https://github.com/tinkoff-ai/etna/pull/1180))


Changelog shouldn't be like this. In actual changelog for automl-2.0 we have already added 2.0 release section. You should fix this.
May be the easiest way is to copy actual changelog and add only your changes.

Mr-Geekman · 2023-05-16T08:21:07Z

CHANGELOG.md

@@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## Unreleased
 ### Added
+- Forecast decomposition for `SeasonalMovingAverageModel`([#1180](https://github.com/tinkoff-ai/etna/pull/1180))


Same for any other change for other classes that is unrelated to the task.

tests/test_auto/test_auto.py

GooseIt · 2023-05-16T09:03:11Z

I'll muster the willpower and finish the task, but thank you for proposing resignation.
I'll report this evening.

GooseIt · 2023-05-16T19:50:45Z

This is not final version, I will remove prints and make lint.

GooseIt · 2023-05-16T20:17:42Z

Last commit is ready to being reviewed

Mr-Geekman

I gave one comment. You can make it or decide to finish and I'll do that after merge.

Mr-Geekman · 2023-05-17T08:17:10Z

etna/auto/auto.py

+
+    def __init__(
+        self,
+        pipeline: Pipeline,


We can try to replace Pipeline here with AbstractPipeline. Because this class isn't specific for the Pipeline. Other methods should be also updated to work with AbstractPipeline.

CHANGELOG.md

GooseIt · 2023-05-17T11:50:21Z

I'll do the replacement, will report tomorrow.

GooseIt · 2023-05-18T19:57:32Z

For hyperparameter suggestion logic in _objective to work, having set_params method is mandatory.

set_params is defined for class BaseMixin in etna/core/mixins.py. Nor AbstractPipeline nor BasePipeline support it.

Should I inherit one of these classes from BaseMixin, and if yes, which one?

Mr-Geekman · 2023-05-19T09:34:44Z

As I can see, BasePipeline already inherits BaseMixin.

GooseIt · 2023-05-19T11:51:21Z

The PR is ready to be reviewed.

etna/auto/auto.py

tests/test_auto/test_tune.py

Mr-Geekman

Look at comments above.

GooseIt · 2023-05-20T21:19:32Z

I've moved all classes in auto.py from Pipeline class to BasePipeline class.

I think that both Tune and Auto classes should be able to work with any user-defined pipeline structure, not only instances of Pipeline class.

Mr-Geekman · 2023-05-21T19:15:21Z

I'm merging it.

GooseIt · 2023-05-22T10:40:23Z

Thank you for bearing with me during this issue's completion.

I'm grateful for your assistance and reviewing.

I'll take a break from committing into Etna for a while. I don't know if or when I'll come back, but if I do, I will be happy to work with you once again.

GooseIt added 5 commits March 29, 2023 16:20

Merge branch 'automl-2.0' of https://github.com/tinkoff-ai/etna into …

6ae3010

…class_tune

First draft, issues yet to fix

623a380

lint

06ca022

redundant_code_cleaned

506cb10

class_tune

8b1d3eb

updated_changelog

6e65595

Mr-Geekman suggested changes Apr 3, 2023

View reviewed changes

Mr-Geekman suggested changes Apr 7, 2023

View reviewed changes

GooseIt added 3 commits April 11, 2023 20:54

not to discard changes

634eef6

not to discard changes

5bad9e4

not to discard changes

c60c582

Mr-Geekman approved these changes Apr 12, 2023

View reviewed changes

Mr-Geekman suggested changes Apr 12, 2023

View reviewed changes

GooseIt added 3 commits May 15, 2023 19:21

tune_init_optuna_test_added

9a56be8

relative_params_fix

4514ba0

Merge branch 'automl-2.0' of https://github.com/tinkoff-ai/etna into …

29281b3

…class_tune

Mr-Geekman self-requested a review May 15, 2023 18:48

Mr-Geekman suggested changes May 16, 2023

View reviewed changes

unrelevant_changes_left

29aac0a

Mr-Geekman self-requested a review May 16, 2023 20:14

code quality updates

5e80ba2

Mr-Geekman approved these changes May 17, 2023

View reviewed changes

Pipeline to BasePipeline

231af07

Mr-Geekman reviewed May 19, 2023

View reviewed changes

etna/auto/auto.py Outdated Show resolved Hide resolved

Mr-Geekman reviewed May 19, 2023

View reviewed changes

etna/auto/auto.py Outdated Show resolved Hide resolved

Mr-Geekman reviewed May 19, 2023

View reviewed changes

etna/auto/auto.py Outdated Show resolved Hide resolved

Mr-Geekman reviewed May 19, 2023

View reviewed changes

tests/test_auto/test_tune.py Outdated Show resolved Hide resolved

Mr-Geekman suggested changes May 19, 2023

View reviewed changes

Pipeline to BasePipeline

ec97125

Mr-Geekman approved these changes May 21, 2023

View reviewed changes

Mr-Geekman merged commit f4b50fe into tinkoff-ai:automl-2.0 May 21, 2023

Mr-Geekman mentioned this pull request May 21, 2023

class Tune #1024

Closed

Class Tune #1200

Class Tune #1200

Conversation

GooseIt commented Apr 2, 2023 • edited Loading

Before submitting (must do checklist)

Proposed Changes

Closing issues

GooseIt commented Apr 2, 2023 • edited Loading

codecov-commenter commented Apr 2, 2023 • edited Loading

Codecov Report

GooseIt commented Apr 2, 2023

Mr-Geekman commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mr-Geekman commented Apr 3, 2023

Mr-Geekman commented Apr 6, 2023

GooseIt commented Apr 6, 2023

GooseIt commented Apr 9, 2023

GooseIt commented Apr 11, 2023

Mr-Geekman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mr-Geekman left a comment

Choose a reason for hiding this comment

GooseIt commented Apr 12, 2023

Mr-Geekman commented Apr 12, 2023

GooseIt commented Apr 12, 2023 • edited Loading

Mr-Geekman commented Apr 12, 2023

Mr-Geekman commented Apr 12, 2023

Mr-Geekman commented Apr 16, 2023

GooseIt commented Apr 20, 2023

Mr-Geekman commented Apr 25, 2023

GooseIt commented May 15, 2023

Mr-Geekman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GooseIt commented May 16, 2023

GooseIt commented May 16, 2023 • edited Loading

GooseIt commented May 16, 2023

Mr-Geekman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GooseIt commented May 17, 2023

GooseIt commented May 18, 2023 • edited Loading

Mr-Geekman commented May 19, 2023

GooseIt commented May 19, 2023

Mr-Geekman left a comment

Choose a reason for hiding this comment

GooseIt commented May 20, 2023

Mr-Geekman commented May 21, 2023

GooseIt commented May 22, 2023

GooseIt commented Apr 2, 2023 •

edited

Loading

GooseIt commented Apr 2, 2023 •

edited

Loading

codecov-commenter commented Apr 2, 2023 •

edited

Loading

GooseIt commented Apr 12, 2023 •

edited

Loading

GooseIt commented May 16, 2023 •

edited

Loading

GooseIt commented May 18, 2023 •

edited

Loading