Add S3FileLogger and LocalFileLogger #372

Mr-Geekman · 2021-12-10T13:25:47Z

IMPORTANT: Please do not create a Pull Request without creating an issue first.

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?

Type of Change

Examples / docs / tutorials / contributors update
Bug fix (non-breaking change which fixes an issue)
Improvement (non-breaking change which improves an existing feature)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Proposed Changes

Look #344.

Related Issue

#344.

Closing issues

Closes #344.

# Conflicts: # CHANGELOG.md

…s.getenv

codecov-commenter · 2021-12-10T14:53:34Z

Codecov Report

Merging #372 (5d1bd81) into master (e4e4b31) will decrease coverage by 0.31%.
The diff coverage is 72.48%.

@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
- Coverage   87.47%   87.16%   -0.32%     
==========================================
  Files          97       98       +1     
  Lines        4736     4908     +172     
==========================================
+ Hits         4143     4278     +135     
- Misses        593      630      +37

Impacted Files	Coverage Δ
etna/loggers/console_logger.py	`89.65% <ø> (ø)`
etna/loggers/wandb_logger.py	`34.61% <12.50%> (+2.43%)`	⬆️
etna/loggers/file_logger.py	`73.05% <73.05%> (ø)`
etna/loggers/__init__.py	`100.00% <100.00%> (ø)`
etna/loggers/base.py	`95.31% <100.00%> (+1.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4e4b31...5d1bd81. Read the comment docs.

martins0n · 2021-12-13T08:05:05Z

pyproject.toml

@@ -52,6 +52,8 @@ saxpy = "^1.0.1-dev167"
 hydra-slayer = "^0.2.0"
 typer = "^0.4.0"
 omegaconf = "^2.1.1"
+holidays = "^0.11.3"
+boto3 = "^1.20.23"


It's better to limit with older versions maybe three or more year old, or use "*".
To avoid dependencies problem.

martins0n · 2021-12-13T08:16:03Z

etna/loggers/file_logger.py

+        self._current_experiment_folder = self.experiment_folder.joinpath(f"{job_type}_{group}")
+        self._current_experiment_folder.mkdir()
+
+    def _save_table(self, table: pd.DataFrame, name: str):


Let's add gzip flag while logger init

martins0n · 2021-12-13T08:18:06Z

etna/loggers/file_logger.py

+        """
+        super().__init__()
+        if not os.path.isdir(experiments_folder):
+            raise ValueError(f"Folder {experiments_folder} doesn't exist")


it seems we can create folder if it doesn't exist without troubles

martins0n · 2021-12-13T08:21:01Z

etna/loggers/file_logger.py

+            raise ValueError("You should start experiment before using log_backtest_run or log_backtest_metrics")
+        filename = f"{name}.json"
+        with open(self._current_experiment_folder.joinpath(filename), "w") as ouf:
+            json.dump(dictionary, ouf)


We should catch exceptions if some objects not serializable

martins0n · 2021-12-13T09:43:24Z

etna/loggers/file_logger.py

+        metrics.columns = ["segment"] + columns_name
+
+        try:
+            self._save_table(metrics, "metrics")


Metrics has strange columns for some reasons

segment,segment,MAE,MSE,MAPE,SMAPE,R2 0,segment_1,7.763682014880712,63.082959455152306,70.50005183059184,110.20437418869618,-27.28124460927375 1,segment_2,6.169644589069107,39.57608243305653,3.397196323227341e+17,165.08659522722252,-20.297610322998022 2,segment_3,4.148008953160193,26.044276348095234,2.2313460234094784e+17,163.44802769939656,-10.475877270195646 3,segment_8,4.1515436266736705,23.73210099641872,33.21865137226771,42.703159606591534,-8.449524500405012 4,segment_0,1.2761743904371403,2.145912681208096,5.64154880927096,5.855982803150363,-1.2013605113854 5,segment_7,4.802545981302062,24.85181975228773,2.956777063102111e+17,90.2986826786166,-21.374242621538958 6,segment_6,4.951371383445379,25.260573194944055,1.6948198758644096e+18,34.42165015381447,-31.770833878003458 7,segment_9,2.7972933788078693,10.25362118755356,1.3365994548979092e+18,24.294594073351483,-3.004909993258983 8,segment_4,2.3343613388059667,7.798571395070086,5.050117708467421,5.19826439721232,-0.4185895718027921 9,segment_5,3.631983273631512,14.911636014941061,1.6744240388728714e+18,19.535007065540476,-19.032146156809308

martins0n · 2021-12-13T09:56:00Z

etna/loggers/file_logger.py

+DATETIME_FORMAT = "%Y-%m-%dT%H-%M-%S"
+
+
+class BaseFileLogger(BaseLogger):


Let's add config in init like in Wandblogger -- to save experiments params and pipeline.

martins0n · 2021-12-13T09:58:37Z

etna/loggers/file_logger.py

+    def _check_bucket(self):
+        try:
+            self.s3_client.head_bucket(Bucket=self.bucket)
+        except ParamValidationError:


I have another error type:

File "/home/marti/projects/etna-1/etna/loggers/file_logger.py", line 308, in _check_bucket self.s3_client.head_bucket(Bucket=self.bucket) File "/home/marti/.cache/pypoetry/virtualenvs/etna-hkIKRVwg-py3.8/lib/python3.8/site-packages/botocore/client.py", line 391, in _api_call return self._make_api_call(operation_name, kwargs) File "/home/marti/.cache/pypoetry/virtualenvs/etna-hkIKRVwg-py3.8/lib/python3.8/site-packages/botocore/client.py", line 719, in _make_api_call raise error_class(parsed_response, operation_name) botocore.exceptions.ClientError: An error occurred (404) when calling the HeadBucket operation: Not Found

And that's why proper ValueError is not raised

martins0n · 2021-12-13T09:59:33Z

etna/loggers/file_logger.py

+        # create subfolder for current experiment
+        cur_datetime = datetime.datetime.now()
+        subfolder_name = cur_datetime.strftime(DATETIME_FORMAT)
+        self.experiment_folder = pathlib.Path(self.experiments_folder).joinpath(subfolder_name)


It's better to pathlib.Path(self.experiments_folder).resolve() to get full path of experiment

martins0n · 2021-12-13T10:04:25Z

etna/loggers/file_logger.py

+        group:
+            Specify a group to organize individual runs into a larger experiment.
+        """
+        self._current_experiment_folder = self.experiment_folder.joinpath(f"{job_type}_{group}")


If job_type=None and group=None I'll get invalid folder name

martins0n · 2021-12-13T10:12:46Z

etna/loggers/file_logger.py

+            raise e
+
+        # case for aggregate_metrics=False
+        if "fold_number" in metrics_df.columns:


Can we make it in external function that is common for all loggers?

…nfigs, add clarifications to start_experiment

martins0n · 2021-12-15T06:40:21Z

etna/loggers/file_logger.py

+        # create subfolder for current experiment
+        cur_datetime = datetime.datetime.now()
+        subfolder_name = cur_datetime.strftime(DATETIME_FORMAT)
+        self.experiment_folder = os.path.join(experiments_folder, subfolder_name)


Seems like we should change this path concat too.

martins0n

🥇

d.a.bunin added 4 commits December 8, 2021 16:36

Add LocalFileLogger, fix outdated info in docstrings in other loggers

02c807b

Fix bugs in file_logger, add tests on LocalFileLogger

91c0da0

Add new tests for file_logger

2c9cb34

Add S3FileLogger and tests for it, update tests for LocalFileLogger

34effa9

Mr-Geekman added the enhancement New feature or request label Dec 10, 2021

Mr-Geekman self-assigned this Dec 10, 2021

d.a.bunin added 6 commits December 10, 2021 16:27

Update changelog

376aabd

Merge remote-tracking branch 'origin/master' into issue-344

f791f3e

# Conflicts: # CHANGELOG.md

Add boto3 dependency

2b4e949

Add boto3 dependency

a59ed30

Move holidays dependency to other mandatory dependencies

a3066d9

Change format of the folder for experiment, replace os.environ with o…

f7b1dab

…s.getenv

Fix tests on checking env variables

1c06833

Mr-Geekman marked this pull request as ready for review December 10, 2021 15:54

martins0n suggested changes Dec 13, 2021

View reviewed changes

d.a.bunin added 11 commits December 13, 2021 14:07

Fix lower boto3 version

e83903f

Add gzip flag

7adb742

Create experiments folder in FileLogger if it doesn't exist

bb581fc

Catch errors inside logging

ad39010

Fix config type in WandbLogger, add logging config to file_logger

b9b0d45

Fix bug with modifyin metrics_df in WandbLogger and BaseFileLogger

500ce34

Fix raising error on not found bucket

34fb5ed

Add resolve to a path in LocalFileLogger

dd34e9b

Make start_experiment work with None values, change path of saving co…

4639bde

…nfigs, add clarifications to start_experiment

Extract common code from WandbLogger and BaseFileLogger into a function

0cff9c4

Add description to file_logger classes

307cd04

martins0n reviewed Dec 15, 2021

View reviewed changes

d.a.bunin added 2 commits December 15, 2021 10:04

Fix path.join to concatenation

2c84f13

Merge remote-tracking branch 'origin/master' into issue-344

5d1bd81

martins0n approved these changes Dec 15, 2021

View reviewed changes

martins0n merged commit 39f63c3 into master Dec 15, 2021

martins0n deleted the issue-344 branch December 15, 2021 07:29

martins0n pushed a commit that referenced this pull request Dec 23, 2021

Add S3FileLogger and LocalFileLogger (#372)

98d0c6a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add S3FileLogger and LocalFileLogger #372

Add S3FileLogger and LocalFileLogger #372

Mr-Geekman commented Dec 10, 2021 •

edited

Loading

codecov-commenter commented Dec 10, 2021 •

edited

Loading

martins0n Dec 13, 2021

martins0n Dec 13, 2021

martins0n Dec 13, 2021

martins0n Dec 13, 2021

martins0n Dec 13, 2021

martins0n Dec 13, 2021

martins0n Dec 13, 2021

martins0n Dec 13, 2021

martins0n Dec 13, 2021

martins0n Dec 13, 2021

martins0n Dec 15, 2021

martins0n left a comment

		DATETIME_FORMAT = "%Y-%m-%dT%H-%M-%S"


		class BaseFileLogger(BaseLogger):

Add S3FileLogger and LocalFileLogger #372

Add S3FileLogger and LocalFileLogger #372

Conversation

Mr-Geekman commented Dec 10, 2021 • edited Loading

Before submitting (must do checklist)

Type of Change

Proposed Changes

Related Issue

Closing issues

codecov-commenter commented Dec 10, 2021 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

martins0n left a comment

Choose a reason for hiding this comment

Mr-Geekman commented Dec 10, 2021 •

edited

Loading

codecov-commenter commented Dec 10, 2021 •

edited

Loading