Add bot comments in app.cfg.example #170

jonas-lq · 2023-03-27T13:47:14Z

No description provided.

boegel · 2023-03-28T07:12:51Z

app.cfg.example

@@ -134,3 +134,24 @@ poll_interval = 60

 # full path to the command for manipulating existing jobs
 scontrol_command = /usr/bin/scontrol
+
+
+[submitted_job_comments]


add a comment above here to indicate that some of these comments should not be changed because we have regular expression patterns in the code that match them

trz42

Looks already quite good. Basic functionality is there.

The PR needs to be updated with the head of the EESSI/eessi-bot-software-layer repository (some conflicts for tasks/build.py).

Wonder a bit if variable names could be made more telling (more than comments) and settings could get unique names (a few are named comment ... they are in different sections, but might be confusing still).

When accessing configuration settings and sections we tried to use constants, typically defined at the beginning of a file, e.g.,

eessi-bot-software-layer/tasks/build.py

Line 26 in feb7493

BUILDENV = "buildenv"

and in the code then use these, e.g.,

eessi-bot-software-layer/tasks/build.py

Line 78 in feb7493

buildenv = cfg[BUILDENV]

Benefit of that is that we avoid problems caused by typos and could change the names more easily at a single place (where constants are defined).

Now that we use settings from app.cfg there is a risk that some setting is not defined. Should we add some function that provides a limited check that certain configuration settings are available? Similarly to what is done in EESSI/filesystem-layer#90 with defining required config settings (file scripts/automated_ingestion/automated_ingestion.py lines 17-22) and using them (same file, lines 64-69). At the start the job manager and the event handler could check that the configuration contains the required settings and stops if not.

trz42 · 2023-03-28T10:32:47Z

tasks/build.py

-    job_comment = (f"New job on instance `{app_name}`"
-                   f" for architecture `{arch_name}`"
-                   f" in job dir `{symlink}`\n"
+    comments = config.read_config()["submitted_job_comments"]


Wonder a bit if comments is a good name for the variable. It's more like submit_comment_cfg ???

trz42 · 2023-03-28T10:41:06Z

eessi_bot_job_manager.py

@@ -291,9 +291,10 @@ def process_new_job(self, new_job):
            # (c) add a row to the table
            # add row to status table if we found a comment
            if "comment_id" in new_job:
+                comments = config.read_config()["new_job_comments"]


Wonder a bit if comments is a good name for the variable. It's more like new_job_comment_cfg or put it into the job_manager section [job_manager] ???

trz42 · 2023-03-28T10:43:52Z

eessi_bot_job_manager.py

-                update = "\n|%s|" % dt.strftime("%b %d %X %Z %Y")
-                update += "released|job awaits launch by Slurm scheduler|"
+                update = "\n|%s|released|" % dt.strftime("%b %d %X %Z %Y")
+                update += f"{comments['comment']}|"


comments['comment'] is very generic. Could we come up with names describing their purpose better?

E.g., job_mgr_cfg['new_job_comment'] or job_manager['new_job_comment']

trz42 · 2023-03-28T10:51:11Z

eessi_bot_job_manager.py

+            comments = config.read_config()["running_job_comments"]
+            running_msg = comments['comment'].format(job_id=running_job['jobid'])


See comment about naming variables and configuration settings. Here, something like

job_manager_cfg = config.read_config()["job_manager"] running_msg = job_manager_cfg["running_job_comment"].format...

Might make the code a bit more readable.

trz42 · 2023-03-28T10:55:41Z

eessi_bot_job_manager.py

-                update = "\n|%s|" % dt.strftime("%b %d %X %Z %Y")
-                update += "running|" + running_msg
+                update = f"\n|{dt.strftime('%b %d %X %Z %Y')}|running|"
+                update += f"{running_msg}|"


Nice that you added the missing | !

trz42 · 2023-03-28T10:56:18Z

eessi_bot_job_manager.py

@@ -463,20 +465,17 @@ def process_finished_job(self, finished_job):

        dt = datetime.now(timezone.utc)

+        comments = config.read_config()["finished_job_comments"]


See other comments about variable naming.

trz42 · 2023-03-28T11:27:04Z

eessi_bot_job_manager.py

@@ -463,20 +465,17 @@ def process_finished_job(self, finished_job):

        dt = datetime.now(timezone.utc)

+        comments = config.read_config()["finished_job_comments"]
+        comment_update = f"\n|{dt.strftime('%b %d %X %Z %Y')}|finished|"


Good that you have rearranged the code!

trz42 · 2023-03-28T11:29:40Z

app.cfg.example

+comment = job id `{job_id}` awaits release by job manager 
+
+[new_job_comments]
+comment = job awaits launch by Slurm scheduler 
+
+[running_job_comments]
+comment = job `{job_id}` is running 


The setting comment is used multiple times. It might be a bit confusing. Could we come up with unique names?

trz42

Thanks for the update. A few minor things could be changed. But could be done in a follow up PR. I'll run a test and if it works, I'll just merge it (even without the suggested changes).

trz42 · 2023-04-06T07:53:47Z

eessi_bot_job_manager.py

@@ -43,6 +43,27 @@

 from pyghee.utils import log, error

+AWAITS_LAUCH = "awaits_lauch"


Small typo. Should probably be AWAITS_LAUNCH = "awaits_launch"

trz42 · 2023-04-06T07:57:49Z

app.cfg.example

@@ -138,3 +138,24 @@ poll_interval = 60

 # full path to the command for manipulating existing jobs
 scontrol_command = /usr/bin/scontrol
+
+# variable 'comment' under 'submitted_job_comments' should not be changed as there are regular expression patterns matching it


Suggest a small modification to use the new variable name initial_comment and mentioning "code", e.g., "... should not be changed as the bot's code uses regular expression pattern to identify a matching comment"

trz42 · 2023-04-06T07:58:42Z

app.cfg.example

+awaits_release = job id `{job_id}` awaits release by job manager 
+
+[new_job_comments]
+awaits_lauch = job awaits launch by Slurm scheduler 


Small typo. Should be awaits_launch.

@trz42 Hmm, this typo was not resolved? 🤔

trz42 · 2023-04-06T07:59:49Z

eessi_bot_job_manager.py

+SUCCESS = "success"
+
+REQUIRED_CONFIG = {
+    NEW_JOB_COMMENTS: [AWAITS_LAUCH],


Small typo. Should be AWAITS_LAUNCH.

trz42 · 2023-04-06T08:01:14Z

eessi_bot_job_manager.py

-                update = "\n|%s|" % dt.strftime("%b %d %X %Z %Y")
-                update += "released|job awaits launch by Slurm scheduler|"
+                update = "\n|%s|released|" % dt.strftime("%b %d %X %Z %Y")
+                update += f"{new_job_comments_cfg[AWAITS_LAUCH]}|"


Small typo. Should be AWAITS_LAUNCH.

Addressed.

boegel · 2023-04-11T07:21:37Z

eessi_bot_job_manager.py

+REQUIRED_CONFIG = {
+    NEW_JOB_COMMENTS: [AWAITS_LAUCH],
+    RUNNING_JOB_COMMENTS: [RUNNING_JOB],
+    FINISHED_JOB_COMMENTS: [SUCCESS, FAILURE, NO_SLURM_OUT, SLURM_OUT, MISSING_MODULES,


Do we really need to make this required in the bot configuration file?

Why don't we just fall back to a default comment if the bot isn't configured with a custom comment?

Created #176

jonas-lq added 5 commits March 26, 2023 23:30

Comments are now adjusted from cfg file

a49ebef

Added code brackets in some comments

773f995

Moved comments into app.cfg.example

fbd3065

Updated comment in app.cfg.example

762400c

Removed whitespace

1147f46

boegel requested a review from trz42 March 28, 2023 07:09

boegel previously requested changes Mar 28, 2023

View reviewed changes

Added comment regarding regex in pr comments

796a186

trz42 requested changes Mar 28, 2023

View reviewed changes

jonas-lq marked this pull request as draft March 29, 2023 07:28

jonas-lq added 5 commits March 29, 2023 09:45

Merge branch 'main' into issue#109

25cf777

Fixed hound issues and issues from merging

1aeb7b4

Add cfg settings as constants and settings check

18f3fa0

Resolved hound issues

be64cdc

Fixed pep8 compliance

3d39ee0

jonas-lq marked this pull request as ready for review March 31, 2023 15:22

trz42 reviewed Apr 6, 2023

View reviewed changes

poksumdo mentioned this pull request Apr 6, 2023

Test bot PR170 with main branch of EESSI/software-layer trz42/software-layer#61

Open

trz42 approved these changes Apr 6, 2023

View reviewed changes

trz42 merged commit 6241d41 into EESSI:main Apr 6, 2023

trz42 mentioned this pull request Apr 7, 2023

address minor issues in PR170 #173

Closed

boegel reviewed Apr 11, 2023

View reviewed changes

jonas-lq added a commit to jonas-lq/eessi-bot-software-layer that referenced this pull request Apr 11, 2023

Fixed issues mentioned in EESSI#170

cca788f

This was referenced Apr 11, 2023

Fixed typos from PR170 #175

Merged

Add default pr comments #176

Open

Define PR comment formats/templates in app.cfg #109

Closed

jonas-lq deleted the issue#109 branch April 28, 2023 10:54

boegel added this to the 0.1.0 - initial release milestone Oct 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bot comments in app.cfg.example #170

Add bot comments in app.cfg.example #170

jonas-lq commented Mar 27, 2023

boegel Mar 28, 2023

trz42 left a comment •

edited

Loading

trz42 Mar 28, 2023

trz42 Mar 28, 2023

trz42 Mar 28, 2023

trz42 Mar 28, 2023

trz42 Mar 28, 2023

trz42 Mar 28, 2023

trz42 Mar 28, 2023

trz42 Mar 28, 2023

trz42 left a comment

trz42 Apr 6, 2023

trz42 Apr 7, 2023

trz42 Apr 6, 2023

trz42 Apr 7, 2023

trz42 Apr 6, 2023

boegel Apr 7, 2023

trz42 Apr 7, 2023

trz42 Apr 6, 2023

trz42 Apr 7, 2023

trz42 Apr 6, 2023

trz42 Apr 7, 2023

boegel Apr 11, 2023

jonas-lq Apr 11, 2023

		comments = config.read_config()["running_job_comments"]
		running_msg = comments['comment'].format(job_id=running_job['jobid'])

		@@ -463,20 +465,17 @@ def process_finished_job(self, finished_job):

		dt = datetime.now(timezone.utc)

		comments = config.read_config()["finished_job_comments"]

		@@ -43,6 +43,27 @@

		from pyghee.utils import log, error

		AWAITS_LAUCH = "awaits_lauch"

Add bot comments in app.cfg.example #170

Add bot comments in app.cfg.example #170

Conversation

jonas-lq commented Mar 27, 2023

Choose a reason for hiding this comment

trz42 left a comment • edited Loading

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

trz42 left a 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

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

trz42 left a comment •

edited

Loading