Add all expid_dir_path functions to autosubmit.py, add attached files to emails #1997

kinow · 2024-12-13T01:24:21Z

In GitLab by @isimo00 on Nov 5, 2024, 12:27

All previous definitions all expid_path, tmp_path, log_path and aslogs_path (and similar) have been removed and their calls have been switched for BasicConfig.foo() calls. Impacted tests are mainly affected in terms of adding Mock objects since BasicConfig.foo() functions are not available in autosubmitconfigparser yet. No new tests added.

Resolve merging conflicts
Remove mocked tests and use testing from latest configparser commit until new version is launched

kinow · 2024-12-13T01:24:26Z

In GitLab by @isimo00 on Nov 5, 2024, 12:27

requested review from @kinow

kinow · 2024-12-13T01:24:29Z

In GitLab by @isimo00 on Nov 5, 2024, 12:27

requested review from @dbeltrankyl and removed review request for @kinow

kinow · 2024-12-13T01:24:32Z

In GitLab by @isimo00 on Nov 5, 2024, 12:49

mentioned in commit 4eec259671daf0dcadb2454ac58c7a91dd9b5afe

kinow · 2024-12-13T01:24:35Z

In GitLab by @isimo00 on Nov 5, 2024, 12:49

added 9 commits

1dbf37a0...7a32f96f - 6 commits from branch master
4eec2596 - Add all expid_dir_path functions to autosubmit.py and fix tests !509
daeeaf8f - Add expid paths to error notifications Possible bug in STATUS: RUNNING dependencies #1435
af6e9e63 - Merge branch 'iss1435' of https://earth.bsc.es/gitlab/es/autosubmit into iss1435

Compare with previous version

kinow · 2024-12-13T01:24:38Z

In GitLab by @dbeltrankyl on Nov 5, 2024, 12:54

Commented on autosubmit/notifications/mail_notifier.py line 76

Hello @isimo00,

Thanks for working on it!

I think it should point directly to the exact err and exact out instead of the folder.

Maybe.. we can attach the logs in the mail itself? and/or point to the specific log in GUI? As it can be easily read ( the latter, not sure how to do it without hardcore it FYI @LuiggiTenorioK )

what do you think?

kinow · 2024-12-13T01:24:41Z

In GitLab by @isimo00 on Nov 5, 2024, 12:58

marked the checklist item Resolve merging conflicts as completed

kinow · 2024-12-13T01:24:44Z

In GitLab by @dbeltrankyl on Nov 5, 2024, 12:58

Commented on test/unit/conftest.py line 68

Hello,

Why the change of / per os.path.join? / is used for Path() objects

Maybe we can use Path().joinpath() if you find that / is not readable enough instead of using two different libs

FYI @kinow

kinow · 2024-12-13T01:24:47Z

In GitLab by @dbeltrankyl on Nov 5, 2024, 12:58

Commented on test/unit/conftest.py line 68

In general, I think that @kinow recommended using Path for all ( per example, os.path.exists -> Path().exists() ) , os.makedirs -> Path().mkdir(Parents=True,exists_ok=True) ( didn't double check the syntax) )

edit: There are other similar cases across the changes

kinow · 2024-12-13T01:24:50Z

In GitLab by @dbeltrankyl on Nov 5, 2024, 13:04

Commented on test/unit/conftest.py line 197

Can we use the mocker fixture there?

Instead of using

from unittest import mock

kinow · 2024-12-13T01:24:53Z

In GitLab by @isimo00 on Nov 5, 2024, 13:04

Commented on test/unit/conftest.py line 68

The functions added here (e.g. aslogs_dir = BasicConfig.aslog_dir_mock(expid) in line 50) return str objects created with os.path.join() - see commit in configparser -. I didn't give it much thought and carried on with str instead of PosixPath objects when possible. All of them could be switched back to Path

kinow · 2024-12-13T01:24:56Z

In GitLab by @dbeltrankyl on Nov 5, 2024, 13:06

Thanks @isimo00!

I comment in one stuff to change ( maybe)

and commented on some stuff about the tests. I think we should agree on what to use for the Paths with @kinow , @LuiggiTenorioK mainly to normalize the code

kinow · 2024-12-13T01:24:59Z

In GitLab by @isimo00 on Nov 5, 2024, 13:06

Commented on autosubmit/notifications/mail_notifier.py line 76

You mean adding/pasting the content of the log files in the mail?

kinow · 2024-12-13T01:25:02Z

In GitLab by @dbeltrankyl on Nov 5, 2024, 13:15

Commented on autosubmit/notifications/mail_notifier.py line 76

I mean:

Point to the final err ( $expid/tmp/Log_$expid/$job_name....err) ( and out)
Adding it as an attached file ( see the screenshot ) (perhaps we can compress it before adding it )

kinow · 2024-12-13T01:25:05Z

In GitLab by @LuiggiTenorioK on Nov 5, 2024, 13:34

Commented on autosubmit/notifications/mail_notifier.py line 76

Maybe.. we can attach the logs in the mail itself? and/or point to the specific log in GUI? As it can be easily read ( the latter, not sure how to do it without hardcore it FYI @LuiggiTenorioK )

The GUI shows the latest {expid}/tmp/ASLOGS/****_run.log file. Then, you'll have to append the log into that file if you want to show it.

kinow · 2024-12-13T01:25:08Z

In GitLab by @LuiggiTenorioK on Nov 5, 2024, 13:38

and commented on some stuff about the tests. I think we should agree on what to use for the Paths with @kinow , @LuiggiTenorioK mainly to normalize the code

Agree that using pathlib is better than os.path since is a newer implementation based on OOP and has more options. However, I do find using / less readable than os.path.join so maybe we should use its pathlib equivalent (Path(...).joinpath()) instead.

kinow · 2024-12-13T01:25:11Z

In GitLab by @isimo00 on Nov 5, 2024, 14:35

Started switching back to pathlib in configparser - github's pull request num. 51 - with Path(...).joinpath(). Will extend to autosubmit.

kinow · 2024-12-13T01:25:14Z

In GitLab by @isimo00 on Nov 5, 2024, 15:33

mentioned in commit 867e93ff52b3583772ea6e97371c5dabf5619ea0

kinow · 2024-12-13T01:25:17Z

In GitLab by @isimo00 on Nov 5, 2024, 15:33

added 1 commit

867e93ff - Switch from os.path to pathlib.Path !509 Possible bug in STATUS: RUNNING dependencies #1435

Compare with previous version

kinow · 2024-12-13T01:25:20Z

In GitLab by @isimo00 on Nov 5, 2024, 15:47

Commented on test/unit/conftest.py line 197

changed this line in version 4 of the diff

kinow · 2024-12-13T01:25:23Z

In GitLab by @isimo00 on Nov 5, 2024, 15:47

added 1 commit

298b0cda - Use mocker instead of mock from unittest

Compare with previous version

kinow · 2024-12-13T01:25:26Z

In GitLab by @isimo00 on Nov 5, 2024, 15:55

Commented on test/unit/conftest.py line 68

changed this line in version 5 of the diff

kinow · 2024-12-13T01:25:29Z

In GitLab by @isimo00 on Nov 5, 2024, 15:55

added 1 commit

1ac2fe49 - Fix forgotten Path

Compare with previous version

kinow · 2024-12-13T01:25:32Z

In GitLab by @kinow on Nov 6, 2024, 10:13

Commented on autosubmit/notifications/mail_notifier.py line 76

Not reviewing this one, just going through inbox emails... but maybe we could use a single f-string multiline text without concatenation?

Instead of

  f'some string {vars} \n\n' \
  + f'another string being concatenated {vars} \n etc'

I think this should work too?

f'''some string {vars} \n\n
more text {vars}
etc'''

kinow · 2024-12-13T01:25:35Z

In GitLab by @isimo00 on Nov 26, 2024, 13:18

Commented on autosubmit/notifications/mail_notifier.py line 76

changed this line in version 6 of the diff

kinow · 2024-12-13T01:25:38Z

In GitLab by @isimo00 on Nov 26, 2024, 13:18

added 1 commit

0dab1606 - Add log file attached to mail message

Compare with previous version

kinow · 2024-12-13T01:25:41Z

In GitLab by @isimo00 on Dec 4, 2024, 11:46

added 1 commit

a776f65c - Fix message text

Compare with previous version

kinow · 2024-12-13T01:25:44Z

In GitLab by @isimo00 on Dec 5, 2024, 10:46

mentioned in commit df8c557e5713428f0c8e1afca942081ed958e55e

kinow · 2024-12-13T01:25:47Z

In GitLab by @isimo00 on Dec 5, 2024, 10:46

added 1 commit

df8c557e - Switch from os.path to pathlib.Path !509 Possible bug in STATUS: RUNNING dependencies #1435

Compare with previous version

kinow · 2024-12-19T09:22:57Z

Maybe we can split it, and leave the pathlib/os.path/BasicConfig changes in the other PR... then this one just adds the email thingy.

isimo00 · 2024-12-20T11:30:58Z

@kinow For some reason I can't tag you as a reviewer, but this is finally done now!

kinow · 2024-12-20T12:50:49Z

@kinow For some reason I can't tag you as a reviewer, but this is finally done now!

It's because I'm the creator of this pull request (due to how the migration script works -- it uses the migration user in the GitHub API calls, as owner).

isimo00 · 2024-12-20T12:57:29Z

Ah I see. Well @kinow , let me know if you think something else is missing / you can't review it or I can tag Luiggi :)

kinow

@isimo00 reviewed most of the changes, pending just mail_notifier.py now (:coffee: break)

test/unit/conftest.py

test/unit/test_catlog.py

test/unit/test_describe.py

test/unit/test_mail.py

autosubmit/notifications/mail_notifier.py

kinow · 2024-12-20T14:17:17Z

autosubmit/notifications/mail_notifier.py


 class MailNotifier:
    def __init__(self, basic_config):
        self.config = basic_config

    def notify_experiment_status(self, exp_id,mail_to,platform):
        message_text = self._generate_message_experiment_status(exp_id, platform)
-        message = MIMEText(message_text)
+        message = MIMEMultipart() 


We need to confirm if this is the best option. I remember the discussion, but I didn't have time to ponder about it, sorry.

@mcastril, here we have a few options, basically,

we send a notification as we already do, but include the location of the logs in the VM/hub/somewhere

we attach the complete log as-is into the email body

we compress and then attach the logs (and delete the compressed archive as the original logs will remain anyway) into the email body

I think I am more included towards 1) to simplify or a 1.5) option, where we let users choose whether they want the location or the attachment, but I had seen a comment about doing the option 2), which is implemented here.

WDYT, Miguel?

@isimo00 from today's AS meeting:

Let's compress the file by default

We don't have to worry about extremely large files as we are only attaching _run logs (shouldn't be in the GB, mayyyybe MB, more likely b/kb)

I will create separate issues for toggle/flags that disable attachment or restrict the attachment size 👍

So if you add the compression here by default, we can review & merge it :-)

Added to 4.1.13 backlog, #2050

autosubmit/notifications/mail_notifier.py

isimo00 · 2024-12-20T15:52:33Z

Thank you for the extensive review @kinow! I've taken it as a sign to finally start using pylint and autopep8. I've marked as resolved the comments for which I've already followed your suggestions so far, will continue on Monday :)

kinow · 2024-12-20T16:04:16Z

Thank you for the extensive review @kinow! I've taken it as a sign to finally start using pylint and autopep8. I've marked as resolved the comments for which I've already followed your suggestions so far, will continue on Monday :)

I think Luiggi is using Ruff. In the end, these tools in Python tend to follow more or less the same set of rules, based on pep8 (normally pep8 is a subset of all lint rules, as these tools add more on top of it). So that's a good start! But maybe check with Luiggi if it would be better if you used ruff too. This is up to discussion too, but ruff is also used in other BSC projects (and DestinE workflow, GSV, etc.) so there is a good case for us to use that too.

And thank you for the great PR with tests, and for splitting the change in the pathlib that would require more test coverage.

kinow

Finished reviewing it, @isimo00 . I think we can wait for others (especially Miguel) to have time to review the part about attaching logs to the email body. But otherwise the code looks good to me. I added a couple of comments now after reading it one last time. The part about BaseException was already in the code, but maybe we can improve it here. Thanks!!

autosubmit/notifications/mail_notifier.py

test/unit/conftest.py

test/unit/test_job_pytest.py

autosubmit/notifications/mail_notifier.py

test/unit/test_mail.py

autosubmit/notifications/mail_notifier.py

kinow

There are some changes appearing here that I am not sure if they were part of your initial change. Maybe something went wrong rebasing?

There are also some conflicts...

isimo00 · 2024-12-27T12:21:05Z

I noticed that too and undid the rebase, thanks @kinow !

kinow · 2024-12-27T13:11:19Z

Great! @LuiggiTenorioK merged the PR that fixes the rocrate tests. I think if you rebase it (assuming your rebase was done earlier) that should fix the build here 🤞

All previous definitions all expid_path, tmp_path, log_path and aslogs_path (and similar) have been removed and their calls have been switched for BasicConfig.foo() calls. Impacted tests are mainly affected in terms of adding Mock objects since BasicConfig.foo() functions are not available in autosubmitconfigparser yet. No new tests added

autosubmit/notifications/mail_notifier.py

isimo00 · 2025-01-10T10:30:52Z

@kinow I believe this is ready to merge, if you'd like to do a final revision I'll be checking the issue for further comments

kinow · 2025-01-10T18:37:06Z

@kinow I believe this is ready to merge, if you'd like to do a final revision I'll be checking the issue for further comments

Excellent @isimo00 ! I'm leaving it unread in my GH notifications, planning to review it next Monday! 💪

Thank you!

kinow added this to the Autosubmit 4.1.12 milestone Dec 13, 2024

kinow assigned isimo00 Dec 13, 2024

isimo00 force-pushed the iss1435 branch from 68f491a to 04a710a Compare December 20, 2024 11:26

isimo00 changed the title ~~Draft: Add all expid_dir_path functions to autosubmit.py and fix tests~~ Add all expid_dir_path functions to autosubmit.py, add attached files to emails Dec 20, 2024

isimo00 removed the working on Someone is working on it label Dec 20, 2024

isimo00 mentioned this pull request Dec 20, 2024

Improve test coverage #2045

Open

12 tasks

kinow commented Dec 20, 2024

View reviewed changes

kinow commented Dec 27, 2024

View reviewed changes

autosubmit/notifications/mail_notifier.py Outdated Show resolved Hide resolved

isimo00 force-pushed the iss1435 branch from e13831a to 185336e Compare December 27, 2024 12:06

kinow commented Dec 27, 2024

View reviewed changes

isimo00 force-pushed the iss1435 branch from 185336e to 04a710a Compare December 27, 2024 12:17

isimo00 force-pushed the iss1435 branch from 04a710a to f067bc3 Compare December 27, 2024 13:33

This was referenced Jan 2, 2025

Add configuration options for mail notification #2050

Open

Path to logs on email notification #1434

Open

isimo00 mentioned this pull request Jan 8, 2025

Email notification on finished chunk or/and experiment #2055

Open

Add compression of log files and one test

3e47faf

kinow commented Jan 9, 2025

View reviewed changes

autosubmit/notifications/mail_notifier.py Outdated Show resolved Hide resolved

Irene Simo Munoz added 2 commits January 9, 2025 16:39

Default to last log file and two more test cases

f0d13ba

Add more tests

d5da9d3

Add all expid_dir_path functions to autosubmit.py, add attached files to emails #1997

Are you sure you want to change the base?

Add all expid_dir_path functions to autosubmit.py, add attached files to emails #1997

Conversation

kinow commented Dec 13, 2024 • edited by isimo00 Loading

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 13, 2024

kinow commented Dec 19, 2024

isimo00 commented Dec 20, 2024

kinow commented Dec 20, 2024

isimo00 commented Dec 20, 2024

kinow left a comment

Choose a reason for hiding this comment

kinow Dec 20, 2024

Choose a reason for hiding this comment

kinow Jan 2, 2025

Choose a reason for hiding this comment

kinow Jan 2, 2025

Choose a reason for hiding this comment

isimo00 commented Dec 20, 2024

kinow commented Dec 20, 2024 • edited Loading

kinow left a comment

Choose a reason for hiding this comment

kinow left a comment

Choose a reason for hiding this comment

isimo00 commented Dec 27, 2024

kinow commented Dec 27, 2024

isimo00 commented Jan 10, 2025

kinow commented Jan 10, 2025

kinow commented Dec 13, 2024 •

edited by isimo00

Loading

kinow commented Dec 20, 2024 •

edited

Loading