[METEOR-1737][feat] bug-reporter: also get microscope files from the last run #3337
Conversation
This helps finding the file back.
📝 WalkthroughWalkthroughA new helper method Sequence Diagram(s)sequenceDiagram
participant Caller
participant OdemisBugreporter
participant Filesystem
participant Archiver
participant Logger
Caller->>OdemisBugreporter: compress_files(logfile, selected_models, configpath)
OdemisBugreporter->>OdemisBugreporter: collect root-level model files
OdemisBugreporter->>OdemisBugreporter: call _get_last_model_dir_from_log(logfile)
OdemisBugreporter->>Filesystem: read logfile
Filesystem-->>OdemisBugreporter: logfile contents
OdemisBugreporter-->>Logger: debug/info (regex/grep results)
OdemisBugreporter->>Filesystem: list CONFIGPATH for *.odm.yaml and *.tsv
Filesystem-->>OdemisBugreporter: config_model_files
OdemisBugreporter->>Filesystem: list last_model_dir for model files
Filesystem-->>OdemisBugreporter: last_model_files (or empty)
OdemisBugreporter-->>Logger: warn about missing files (if any)
OdemisBugreporter->>Archiver: add root files, add config-mic-files/, add last-mic-files/
Archiver-->>Caller: archive created
sequenceDiagram
participant Odemisd
participant Filesystem
participant Logger
Odemisd->>Odemisd: during init, access self._model.name
Odemisd->>Filesystem: resolve absolute path (os.path.abspath)
Filesystem-->>Odemisd: absolute path
Odemisd-->>Logger: debug log of absolute model instantiation path
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request enhances the bug reporter to capture microscope configuration files from the directory of the last model file that was actually used during runtime, in addition to the files from the standard configuration path.
Changes:
- Modified backend logging to include absolute path of model instantiation file for better parsing
- Added functionality to parse log files and extract the last used model directory
- Reorganized bug report archive structure with separate subfolders for config files and last-run model files
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/odemis/odemisd/main.py | Changed logging statement to output absolute path of model file instead of relative path, enabling the bug reporter to parse it correctly |
| src/bugreporter/odemis_bugreporter.py | Added _get_last_model_dir_from_log method to parse logs for the last model directory; refactored compress_files to collect and organize model files from both CONFIGPATH and the last-run directory into separate archive subfolders |
| if last_model_dir and os.path.isdir(last_model_dir): | ||
| logging.debug("Adding model files from last used model directory: %s", last_model_dir) | ||
| last_model_files.update(glob(os.path.join(last_model_dir, '*.odm.yaml'))) | ||
| last_model_files.update(glob(os.path.join(last_model_dir, '*.tsv'))) |
There was a problem hiding this comment.
If the last model directory (from the log) happens to be the same as CONFIGPATH, the same files will be collected twice - once in config_model_files and once in last_model_files. While they will be stored in different subfolders in the archive, this creates redundancy. Consider checking if last_model_dir equals odemis_config['CONFIGPATH'] and skipping the last_model_files collection in that case, or explicitly documenting this behavior.
| if last_model_dir and os.path.isdir(last_model_dir): | |
| logging.debug("Adding model files from last used model directory: %s", last_model_dir) | |
| last_model_files.update(glob(os.path.join(last_model_dir, '*.odm.yaml'))) | |
| last_model_files.update(glob(os.path.join(last_model_dir, '*.tsv'))) | |
| config_path = odemis_config.get('CONFIGPATH') | |
| if last_model_dir and os.path.isdir(last_model_dir): | |
| # Avoid collecting the same model files twice if the last model directory | |
| # is the same as the CONFIGPATH directory. | |
| if config_path and os.path.abspath(last_model_dir) == os.path.abspath(config_path): | |
| logging.debug( | |
| "Last model directory %s is the same as CONFIGPATH; " | |
| "skipping duplicate model file collection.", | |
| last_model_dir, | |
| ) | |
| else: | |
| logging.debug( | |
| "Adding model files from last used model directory: %s", | |
| last_model_dir, | |
| ) | |
| last_model_files.update(glob(os.path.join(last_model_dir, '*.odm.yaml'))) | |
| last_model_files.update(glob(os.path.join(last_model_dir, '*.tsv'))) |
If the user started odemis via the terminal, with the microscope file as parameter, we might not pick up the right microscope file. This is pretty common during installation. So let's try to pick it too. Now save microsocpe files from 3 places: * MODEL -> in the root * CONFIGPATH -> in a sub-folder config-mic-files * last used -> in a sub-folder last-mic-files
pieleric
force-pushed
the
feat-bug-reporter-also-get-microscope-files-from-the-last-run
branch
from
2aeb58c to
615296e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/bugreporter/odemis_bugreporter.py`:
- Around line 284-320: The _get_last_model_dir_from_log function currently
spawns subprocess.check_output with grep; instead, open logfile directly (after
the existing os.path.exists check), read lines in Python, filter for lines
matching the same regex r'model instantiation file is:\s+(/.+\.odm\.yaml)' and
take the last match to compute model_dir via os.path.dirname(model_path),
returning it or None if not found; replace the broad except Exception handler
with specific file I/O error handling (IOError/OSError) and a subprocess removal
(no external grep invocation), and keep logging.debug/logging.warning messages
consistent around missing files or parse failures.
| def _get_last_model_dir_from_log(self, logfile: str) -> Optional[str]: | ||
| """ | ||
| Parse the log file to find the last model instantiation file and return its directory. | ||
| :param logfile: Path to the log file | ||
| :return: Directory path of the last model file, or None if not found | ||
| """ | ||
| if not os.path.exists(logfile): | ||
| logging.debug("Log file %s does not exist", logfile) | ||
| return None | ||
|
|
||
| try: | ||
| # Use grep to find all lines with model instantiation | ||
| result = subprocess.check_output(['grep', 'model instantiation file is:', logfile], | ||
| stderr=subprocess.DEVNULL) | ||
| log_lines = result.decode('utf-8', errors='ignore').splitlines() | ||
|
|
||
| if not log_lines: | ||
| logging.debug("No model instantiation found in log file") | ||
| return None | ||
|
|
||
| # Take the last matching line | ||
| last_line = log_lines[-1] | ||
| match = re.search(r'model instantiation file is:\s+(/.+\.odm\.yaml)', last_line) | ||
| if match: | ||
| model_path = match.group(1).strip() | ||
| model_dir = os.path.dirname(model_path) | ||
| logging.debug("Found last model directory: %s", model_dir) | ||
| return model_dir | ||
|
|
||
| logging.debug("No model path found in last matching line") | ||
| return None | ||
| except subprocess.CalledProcessError: | ||
| # grep returns non-zero exit code when no matches found | ||
| logging.debug("No model instantiation found in log file") | ||
| return None | ||
| except Exception as ex: | ||
| logging.warning("Failed to parse log file %s: %s", logfile, ex) |
There was a problem hiding this comment.
Avoid spawning external grep for log parsing (security & portability).
logfile comes from config; invoking grep via PATH resolution can be abused (or simply fail) and a path starting with - can be treated as an option. This also makes the code dependent on the presence of grep. Parse the file directly in Python and narrow the exception to file I/O errors.
🛠️ Suggested safer in-Python parsing
- try:
- # Use grep to find all lines with model instantiation
- result = subprocess.check_output(['grep', 'model instantiation file is:', logfile],
- stderr=subprocess.DEVNULL)
- log_lines = result.decode('utf-8', errors='ignore').splitlines()
-
- if not log_lines:
- logging.debug("No model instantiation found in log file")
- return None
-
- # Take the last matching line
- last_line = log_lines[-1]
- match = re.search(r'model instantiation file is:\s+(/.+\.odm\.yaml)', last_line)
+ try:
+ last_line = None
+ with open(logfile, "r", encoding="utf-8", errors="ignore") as fh:
+ for line in fh:
+ if "model instantiation file is:" in line:
+ last_line = line
+ if not last_line:
+ logging.debug("No model instantiation found in log file")
+ return None
+ match = re.search(r'model instantiation file is:\s+(/.+\.odm\.yaml)', last_line)
if match:
model_path = match.group(1).strip()
model_dir = os.path.dirname(model_path)
logging.debug("Found last model directory: %s", model_dir)
return model_dir
-
- logging.debug("No model path found in last matching line")
- return None
- except subprocess.CalledProcessError:
- # grep returns non-zero exit code when no matches found
- logging.debug("No model instantiation found in log file")
- return None
- except Exception as ex:
- logging.warning("Failed to parse log file %s: %s", logfile, ex)
+ logging.debug("No model path found in last matching line")
+ return None
+ except OSError as ex:
+ logging.warning("Failed to read log file %s: %s", logfile, ex)
return None🧰 Tools
🪛 Ruff (0.14.14)
[error] 296-296: subprocess call: check for execution of untrusted input
(S603)
[error] 296-296: Starting a process with a partial executable path
(S607)
[warning] 314-314: Consider moving this statement to an else block
(TRY300)
[warning] 319-319: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@src/bugreporter/odemis_bugreporter.py` around lines 284 - 320, The
_get_last_model_dir_from_log function currently spawns subprocess.check_output
with grep; instead, open logfile directly (after the existing os.path.exists
check), read lines in Python, filter for lines matching the same regex r'model
instantiation file is:\s+(/.+\.odm\.yaml)' and take the last match to compute
model_dir via os.path.dirname(model_path), returning it or None if not found;
replace the broad except Exception handler with specific file I/O error handling
(IOError/OSError) and a subprocess removal (no external grep invocation), and
keep logging.debug/logging.warning messages consistent around missing files or
parse failures.
1 participant
If the user started odemis via the terminal, with the microscope file as parameter, we might not pick up the right microscope file. This is pretty common during installation. So let's try to pick it too.
Now save microsocpe files from 3 places:
* MODEL -> in the root
* CONFIGPATH -> in a sub-folder config-mic-files
* last used -> in a sub-folder last-mic-files
Also include the *tsv files (which contain hardware config)
Also report the full path of the microscope file in the log, so that it's actually possible to know where the last file was.