Skip to content

Conversation

@Alxiice
Copy link
Contributor

@Alxiice Alxiice commented Aug 21, 2025

Description

  • Adding prepareLogger and restoreLogger on the node, that can be used to setup and restore the root logger
  • Update the LogManager to handle more than only the chunks
  • Add trace log level to logging with an inferior level than debug (debug is 10 by default so trace would be 5 with this PR)
  • Add new internal input "logLevel" to setup default logging level for the node

Now here's how we can use it

  1. Using chunk.logger (previous way)
class TestNode(desc.Node):
    def processChunk(self, chunk):
        chunk.logManager.start(chunk.node.verboseLevel.value)
        chunk.logger.info   (f"Message with info    level ({chunk.logger})")
        chunk.logger.warning(f"Message with warning level ({chunk.logger})")
        chunk.logger.error  (f"Message with error   level ({chunk.logger})")
        chunk.logManager.end()
image
  1. Using logging
import logging
logger = logging.getLogger("TestNode2")

def print_info():
    logger.info   (f"Message with info    level ({logger})")
    logger.warning(f"Message with warning level ({logger})")
    logger.error  (f"Message with error   level ({logger})")

class TestNode2(desc.Node):
    def processChunk(self, chunk):
        logger.setLevel(chunk.node.verboseLevel.value.upper())
        print_info()
image

@codecov
Copy link

codecov bot commented Aug 21, 2025

Codecov Report

❌ Patch coverage is 92.36111% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.02%. Comparing base (e704e5b) to head (4e8bb22).
⚠️ Report is 11 commits behind head on develop.

Files with missing lines Patch % Lines
meshroom/core/node.py 86.79% 7 Missing ⚠️
meshroom/__init__.py 71.42% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2845      +/-   ##
===========================================
+ Coverage    77.79%   79.02%   +1.22%     
===========================================
  Files           50       51       +1     
  Lines         6725     6856     +131     
===========================================
+ Hits          5232     5418     +186     
+ Misses        1493     1438      -55     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fabiencastan
Copy link
Member

I would like to simply it a bit more, like this:

import logging

def print_info():
    logging.info   (f"Message with info    level ({logger})")
    logging.warning(f"Message with warning level ({logger})")
    logging.error  (f"Message with error   level ({logger})")

class TestNode2(desc.Node):
    def processChunk(self, chunk):
        logging.getLogger().setLevel(chunk.node.verboseLevel.value.upper())
        print_info()

So if we call any 3rd party python code, the logging behavior will be correct.

And maybe in another PR, move the verbose param as a default param, so we can configure it by default.

@Alxiice Alxiice force-pushed the dev/update_chunk_logger branch from 1962ed0 to 7d27047 Compare August 25, 2025 08:50
@Alxiice
Copy link
Contributor Author

Alxiice commented Aug 25, 2025

@fabiencastan

So if we call any 3rd party python code, the logging behavior will be correct.

Actually I tested and with the code you provided it works already

And maybe in another PR, move the verbose param as a default param, so we can configure it by default.

You mean as an internalInput ?

@fabiencastan
Copy link
Member

You mean as an internalInput ?

Yes

@Alxiice Alxiice force-pushed the dev/update_chunk_logger branch from 37b551a to 951f46e Compare August 25, 2025 15:26
@cbentejac cbentejac requested a review from Copilot August 25, 2025 15:33

This comment was marked as outdated.

@cbentejac cbentejac requested a review from Copilot August 25, 2025 15:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces comprehensive logging infrastructure improvements to enable plugins to use Python's standard logging module alongside the existing chunk-based logging system. The main goal is to expand logging capabilities beyond just chunk processing to support broader node-level and plugin logging.

  • Refactors the LogManager class to work with any logger and log file, not just chunks
  • Adds a new trace logging level below debug for more granular debugging
  • Introduces prepareLogger/restoreLogger methods on nodes for setting up root logger configuration
  • Adds a new internal input parameter for configuring default node logging levels

Reviewed Changes

Copilot reviewed 5 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/test_compute.py Comprehensive test suite for the new logging functionality with chunked and non-chunked nodes
meshroom/core/nodeFactory.py Prevents conflicts between user inputs and internal inputs by filtering duplicates
meshroom/core/node.py Major refactoring of LogManager and addition of node-level logging methods
meshroom/core/desc/node.py Adds new nodeDefaultLoglevel internal parameter for configuring logging levels
meshroom/init.py Implements custom TRACE logging level and updates logging level mappings

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@Alxiice Alxiice force-pushed the dev/update_chunk_logger branch from 951f46e to c6c8417 Compare August 25, 2025 15:55

This comment was marked as off-topic.

@cbentejac cbentejac added this to the Meshroom 2026.1.0 milestone Sep 1, 2025
Copy link
Contributor

@cbentejac cbentejac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything works as expected, the requested changes are mostly cosmetic.

Comment on lines +178 to +179
# The line below can cause UI issues but at least prevent crashes
internalInputs = {k: v for k, v in self.internalInputs.items() if k not in self.inputs.keys()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary, since the internal input for the logger doesn't have (and isn't likely to ever have) the same name as an input parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be necessary, but I still think this is better to have this protection, because having twice the same keyword arg in the call of Node(...) will always raise an error.

@Alxiice Alxiice force-pushed the dev/update_chunk_logger branch from c6c8417 to 52c4e91 Compare September 5, 2025 14:01
@Alxiice Alxiice force-pushed the dev/update_chunk_logger branch from 7ddb38b to 0529cc1 Compare September 9, 2025 13:42
@Alxiice Alxiice requested a review from cbentejac September 9, 2025 13:55
@cbentejac cbentejac merged commit 26f62b6 into develop Sep 10, 2025
5 checks passed
@cbentejac cbentejac deleted the dev/update_chunk_logger branch September 10, 2025 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants