Skip to content

fix: reduce upload workers to 2#390

Merged
mcembalest merged 2 commits intomainfrom
reduce-upload-workers-to-2
May 16, 2025
Merged

fix: reduce upload workers to 2#390
mcembalest merged 2 commits intomainfrom
reduce-upload-workers-to-2

Conversation

@mcembalest
Copy link
Copy Markdown
Contributor

@mcembalest mcembalest commented May 16, 2025

Important

Reduce upload workers from 10 to 2 in _add_blobs(), _add_data(), and update_maps() in nomic/dataset.py.

  • Concurrency:
    • Reduce num_workers from 10 to 2 in _add_blobs(), _add_data(), and update_maps() in nomic/dataset.py.
  • Functions Affected:
    • _add_blobs(): Handles image blob uploads.
    • _add_data(): Manages low-level data uploads.
    • update_maps(): Deprecated method for updating project maps.

This description was created by Ellipsis for 42e35f0. You can customize this summary. It will automatically update as commits are pushed.

@mcembalest mcembalest requested a review from bmschmidt May 16, 2025 17:19
Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to ab35b93 in 57 seconds. Click for details.
  • Reviewed 34 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. nomic/dataset.py:1422
  • Draft comment:
    Hardcoded worker count (2) is repeated. Consider defining a constant (e.g. DEFAULT_UPLOAD_WORKERS) to avoid magic numbers and ease future changes.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 70% None
2. nomic/dataset.py:1566
  • Draft comment:
    The _add_data function now uses 2 workers instead of 10. To keep things DRY, consider using a shared constant for the upload worker count.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 70% None
3. nomic/dataset.py:1687
  • Draft comment:
    Default parameter 'num_workers' in update_maps is reduced to 2. Consider documenting this change or using a shared constant so that all upload-related methods remain in sync.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 70% None

Workflow ID: wflow_evCebh1YDZeKKarQ

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@mcembalest mcembalest changed the title reduce upload workers to 2 fix: reduce upload workers to 2 May 16, 2025
Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 42e35f0 in 51 seconds. Click for details.
  • Reviewed 17 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. nomic/dataset.py:1566
  • Draft comment:
    The diff removes an explanatory comment and hardcodes num_workers = 2. Consider briefly retaining a rationale for this choice or making it configurable for flexibility.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 70% None

Workflow ID: wflow_EMJnKcJnqMiJvDKV

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Copy Markdown
Member

@bmschmidt bmschmidt left a comment

Choose a reason for hiding this comment

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

LGTM

@mcembalest mcembalest merged commit a040d3d into main May 16, 2025
3 checks passed
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.

2 participants