Better naming for github action runs #175
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
| @@ -10,6 +10,7 @@ on: | |||
|
|
|||
| jobs: | |||
| build: | |||
| name: ${{ matrix.os }} - ${{ matrix.backend }} - ${{ matrix.implementation }} | |||
There was a problem hiding this comment.
In the line above should we replace "build" with something better? May be "rgat-inference-run" ?
| @@ -10,7 +10,7 @@ on: | |||
|
|
|||
| jobs: | |||
| build: | |||
| name: ${{ matrix.os }} - ${{ matrix.backend }} - ${{ matrix.implementation }} | |||
| name: rgat-inference-run | |||
There was a problem hiding this comment.
@sujik18 I was referring to the previous line where "build" is used. Do you know if the "name" field can override the job name "build"? If so, then it is fine.
There was a problem hiding this comment.
@arjunsuresh Yes, I believe that by mentioning name field it will override the "build" as it did work for other tests.
There was a problem hiding this comment.
If you see the action here it still shows "build": https://github.com/mlcommons/mlperf-automations/actions/runs/13100736773
There was a problem hiding this comment.
@arjunsuresh I tried replacing the build value with another job key value, but the test name is still showing as build. You can see it here https://github.com/mlcommons/mlperf-automations/actions/runs/13104065296/workflow
I double-checked for any syntax errors, but there aren't any. Even if it was a GitHub internal issue, it would have also affected other test names, but that isn't the case, so I'm not sure why it's happening.
There was a problem hiding this comment.
@sujik18 if the pull_request_target is specified in the github action, then the action is running from the base branch and so the PR changes won't be reflected there. But you can see for the changes in the github actions with "on:pull_request".
There was a problem hiding this comment.
Sure. The changes are fine for the github actions. Have you seen my comment on the logging part in the github issue?
There was a problem hiding this comment.
Yes, actually by mistake those commits got included with this PR instead of a new one, wrt to logger issue, apart from making the induvial script to utilize global logger, do we also need each script to write the logs in a single mlc-log.txt file,
I guess that would be a better approach for maintaining the logs?
Currently each script file is generating its own log.txt in its own directory. For that we need to change the default value given to the path for setup_logging function call, as the folder directory of mlc seems to have been changed, the current default value seems to be invalid.
There was a problem hiding this comment.
Oh. That's not the expected behaviour. The default log_path should now be the current directory with this commit
There was a problem hiding this comment.
@arjunsuresh so are we maintaining separate log file for each script?
There was a problem hiding this comment.
For each mlc command we have a log file. But if we run them in the same directory the previous one gets overwritten.
| @@ -3,10 +3,12 @@ | |||
| import json | |||
| import shutil | |||
| import subprocess | |||
| import logging | |||
There was a problem hiding this comment.
No, it's not required we should remove the import, as we can directly utilize global logger
Fix for #134