Skip to content

Conversation

reiffd7
Copy link
Contributor

@reiffd7 reiffd7 commented Apr 10, 2025

Description

Implemented a new Depth Estimation workflow block that leverages Depth Anything V2 model to perform depth estimation on images. This block provides a powerful tool for analyzing spatial relationships and creating depth maps from 2D images.

The implementation includes:

  • A robust workflow block manifest with detailed documentation
  • Comprehensive error handling and model management
  • Clear output definitions for depth maps and normalized depth data

The block is implemented in inference/core/workflows/core_steps/models/foundation/depth_estimation/v1.py.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How has this change been tested, please provide a testcase or example of how you tested the change?

The implementation was tested through:

  1. Unit tests for the workflow block manifest and configuration
  2. Integration tests with the model manager
  3. Performance testing on Apple Silicon devices
  4. Testing of error handling scenarios

Any specific deployment considerations

  • Model weights need to be downloaded on first use
  • Memory usage should be monitored for large batch processing

Docs

  • Docs updated? What were the changes:
    • Added comprehensive block documentation with emojis for better readability
    • Documented input/output specifications
    • Added usage examples and performance considerations
    • Included model version compatibility information
    • Added search keywords for better discoverability

"3. Setting the token in your environment: export HUGGING_FACE_HUB_TOKEN=your_token_here\n"
"Or by logging in with: huggingface-cli login"
) from e
print(f"Error initializing depth estimation model: {str(e)}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use logger instead


def __init__(
self, model_id, *args, dtype=None, huggingface_token=HUGGINGFACE_TOKEN, **kwargs
self, *args, dtype=None, huggingface_token=HUGGINGFACE_TOKEN, **kwargs
Copy link
Collaborator

Choose a reason for hiding this comment

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

please do not remove this parameter without agreement from the code owner (probably @probicheaux would be able to judge best).

self.initialize_model()

# Try to initialize model from cache first
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

please do not change base class as that may imply changes to many other models

Copy link
Collaborator

@PawelPeczek-Roboflow PawelPeczek-Roboflow left a comment

Choose a reason for hiding this comment

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

I am worried about changes in transformers base class - could you elaborate why it is needed for this particular model but was not needed for others?

@reiffd7
Copy link
Contributor Author

reiffd7 commented Apr 16, 2025

@PawelPeczek-Roboflow I got rid of structural changes in transformers base class and am passing integration tests.

if model_type not in self.registry_dict:
raise ModelNotRecognisedError(f"Model type not supported: {model_type}")
return self.registry_dict[model_type]
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

am I right that this change does nothing compared to what it was?

Copy link
Collaborator

@PawelPeczek-Roboflow PawelPeczek-Roboflow left a comment

Choose a reason for hiding this comment

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

@PawelPeczek-Roboflow PawelPeczek-Roboflow self-requested a review April 16, 2025 17:33
Copy link
Collaborator

@PawelPeczek-Roboflow PawelPeczek-Roboflow left a comment

Choose a reason for hiding this comment

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

LGTM, but conflicts needs to be resolved

@grzegorz-roboflow / @hansent - since I am done for today, would u approve when @reiffd7 resolves conflicts?

Copy link
Contributor

@hansent hansent left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@hansent hansent left a comment

Choose a reason for hiding this comment

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

formater check fails. run make style to fix

codeflash-ai bot added a commit that referenced this pull request Apr 16, 2025
…h-estimation-workflow-block`)

In order to optimize the performance of the `add_model` and `get_model` methods, we need to minimize the time spent on certain operations and reduce the number of loggings wherever possible.



**Changes made:**

1. In the `get_model` method of `ModelRegistry`, a single `try/except` block replaces the if-check and separate raise to catch `KeyError`, thereby combining lookup and retrieval into a single step. This optimizes the process of fetching the model class and raising the error when necessary.

2. In the `add_model` method of `ModelManager`, logging messages are minimized and combined to reduce the overhead caused by frequent logging. The change reduces unnecessary calls to `logger.debug()`.

**Line profiling improvements:** 
- For `get_model`: Combined lookup and retrieval operations reduce steps.
- For `add_model`: Consolidating `logger.debug()` calls minimizes overhead and redundant operations.
Copy link
Contributor

codeflash-ai bot commented Apr 16, 2025

⚡️ Codeflash found optimizations for this PR

📄 11% (0.11x) speedup for ModelManager.add_model in inference/core/managers/base.py

⏱️ Runtime : 2.13 milliseconds 1.91 millisecond (best of 118 runs)

I created a new dependent PR with the suggested changes. Please review:

If you approve, it will be merged into this PR (branch depth-estimation-workflow-block).

Copy link
Contributor

@hansent hansent left a comment

Choose a reason for hiding this comment

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

LGTM

@PawelPeczek-Roboflow PawelPeczek-Roboflow merged commit e479a82 into main Apr 17, 2025
32 checks passed
@PawelPeczek-Roboflow PawelPeczek-Roboflow deleted the depth-estimation-workflow-block branch April 17, 2025 09:13
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