Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elizabeth/try local install #10

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

eberrigan
Copy link
Owner

@eberrigan eberrigan commented Sep 27, 2024

npm error code 1
npm error path C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3
npm error command failed
npm error command C:\Windows\system32\cmd.exe /d /s /c prebuild-install || node-gyp rebuild --release
npm error copy_builtin_sqlite3
npm error   sqlite3.c
npm error   win_delay_load_hook.cc
npm error   sqlite3.vcxproj -> C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3\build\Release\\sqlite3.lib
npm error cl : command line  warning D9025: overriding '/std:c++20' with '/std:c++17' [C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3\build\better_sqlite3.vcxproj]
npm error   better_sqlite3.cpp
npm error C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3\build\src\util\macros.lzz(150,35): error C2665: 'v8::ObjectTemplate::SetAccessor': no overloaded function could convert all the argument types [C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3\build\better_sqlite3.vcxproj]
npm error   (compiling source file '../src/better_sqlite3.cpp')
npm error       C:\Users\Elizabeth\AppData\Local\node-gyp\Cache\22.9.0\include\node\v8-template.h(1055,8):
npm error       could be 'void v8::ObjectTemplate::SetAccessor(v8::Local<v8::Name>,v8::AccessorNameGetterCallback,v8::AccessorNameSetterCallback,v8::Local<v8::Value>,v8::PropertyAttribute,v8::SideEffectType,v8::SideEffectType)'
npm error           C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3\build\src\util\macros.lzz(150,35):
npm error           'void v8::ObjectTemplate::SetAccessor(v8::Local<v8::Name>,v8::AccessorNameGetterCallback,v8::AccessorNameSetterCallback,v8::Local<v8::Value>,v8::PropertyAttribute,v8::SideEffectType,v8::SideEffectType)': cannot convert argument 2 from 'v8::AccessorGetterCallback' to 'v8::AccessorNameGetterCallback'
npm error               C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3\build\src\util\macros.lzz(152,17):
npm error               This conversion requires a reinterpret_cast, a C-style cast or parenthesized function-style castnpm error       C:\Users\Elizabeth\AppData\Local\node-gyp\Cache\22.9.0\include\node\v8-template.h(1049,8):
npm error       or       'void v8::ObjectTemplate::SetAccessor(v8::Local<v8::String>,v8::AccessorGetterCallback,v8::AccessorSetterCallback,v8::Local<v8::Value>,v8::PropertyAttribute,v8::SideEffectType,v8::SideEffectType)'
npm error           C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3\build\src\util\macros.lzz(150,35):
npm error           'void v8::ObjectTemplate::SetAccessor(v8::Local<v8::String>,v8::AccessorGetterCallback,v8::AccessorSetterCallback,v8::Local<v8::Value>,v8::PropertyAttribute,v8::SideEffectType,v8::SideEffectType)': cannot convert argument 5 from 'v8::AccessControl' to 'v8::PropertyAttribute'
npm error               C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3\build\src\util\macros.lzz(155,36):
npm error               Conversion to enumeration type requires an explicit cast (static_cast, C-style cast or parenthesized function-style cast)
npm error       C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3\build\src\util\macros.lzz(150,35):
npm error       while trying to match the argument list '(v8::Local<v8::String>, v8::AccessorGetterCallback, int, v8::Local<v8::External>, v8::AccessControl, v8::PropertyAttribute)'
npm error
npm error   test_extension.c
npm error   win_delay_load_hook.cc
npm error      Creating library C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3\build\Release\test_extension.lib and object C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3\build\Release\test_extension.exp
npm error   Generating code
npm error   Previous IPDB not found, fall back to full compilation.
npm error   All 3 functions were compiled because no usable IPDB/IOBJ from previous compilation was found.
npm error   Finished generating code
npm error   test_extension.vcxproj -> C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3\build\Release\\test_extension.node
npm error prebuild-install warn install No prebuilt binaries found (target=22.9.0 runtime=node arch=x64 libc= platform=win32)
npm error gyp info it worked if it ends with ok
npm error gyp info using [email protected]
npm error gyp info using [email protected] | win32 | x64
npm error (node:16580) [DEP0060] DeprecationWarning: The `util._extend` API is deprecated. Please use Object.assign() instead.
npm error (Use `node --trace-deprecation ...` to show where the warning was created)
npm error gyp info find Python using Python version 3.8.20 found at "C:\miniforge3\envs\bloom-desktop\python.exe"
npm error gyp info find VS using VS2022 (17.11.35312.102) found at:
npm error gyp info find VS "C:\Program Files\Microsoft Visual Studio\2022\Community"
npm error gyp info find VS run with --verbose for detailed information
npm error gyp info spawn C:\miniforge3\envs\bloom-desktop\python.exe
npm error gyp info spawn args [
npm error gyp info spawn args   'C:\\repos\\bloom-desktop-pilot\\app\\node_modules\\node-gyp\\gyp\\gyp_main.py',
npm error gyp info spawn args   'binding.gyp',
npm error gyp info spawn args   '-f',
npm error gyp info spawn args   'msvs',
npm error gyp info spawn args   '-I',
npm error gyp info spawn args   'C:\\repos\\bloom-desktop-pilot\\app\\node_modules\\better-sqlite3\\build\\config.gypi',npm error gyp info spawn args   '-I',
npm error gyp info spawn args   'C:\\repos\\bloom-desktop-pilot\\app\\node_modules\\node-gyp\\addon.gypi',
npm error gyp info spawn args   '-I',
npm error gyp info spawn args   'C:\\Users\\Elizabeth\\AppData\\Local\\node-gyp\\Cache\\22.9.0\\include\\node\\common.gypi',
npm error gyp info spawn args   '-Dlibrary=shared_library',
npm error gyp info spawn args   '-Dvisibility=default',
npm error gyp info spawn args   '-Dnode_root_dir=C:\\Users\\Elizabeth\\AppData\\Local\\node-gyp\\Cache\\22.9.0',
npm error gyp info spawn args   '-Dnode_gyp_dir=C:\\repos\\bloom-desktop-pilot\\app\\node_modules\\node-gyp',
npm error gyp info spawn args   '-Dnode_lib_file=C:\\\\Users\\\\Elizabeth\\\\AppData\\\\Local\\\\node-gyp\\\\Cache\\\\22.9.0\\\\<(target_arch)\\\\node.lib',
npm error gyp info spawn args   '-Dmodule_root_dir=C:\\repos\\bloom-desktop-pilot\\app\\node_modules\\better-sqlite3',
npm error gyp info spawn args   '-Dnode_engine=v8',
npm error gyp info spawn args   '--depth=.',
npm error gyp info spawn args   '--no-parallel',
npm error gyp info spawn args   '--generator-output',
npm error gyp info spawn args   'C:\\repos\\bloom-desktop-pilot\\app\\node_modules\\better-sqlite3\\build',
npm error gyp info spawn args   '-Goutput_dir=.'
npm error gyp info spawn args ]
npm error gyp info spawn C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Current\Bin\MSBuild.exe
npm error gyp info spawn args [
npm error gyp info spawn args   'build/binding.sln',
npm error gyp info spawn args   '/clp:Verbosity=minimal',
npm error gyp info spawn args   '/nologo',
npm error gyp info spawn args   '/p:Configuration=Release;Platform=x64'
npm error gyp info spawn args ]
npm error gyp ERR! build error
npm error gyp ERR! stack Error: `C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Current\Bin\MSBuild.exe` failed with exit code: 1
npm error gyp ERR! stack     at ChildProcess.onExit (C:\repos\bloom-desktop-pilot\app\node_modules\node-gyp\lib\build.js:203:23)
npm error gyp ERR! stack     at ChildProcess.emit (node:events:519:28)
npm error gyp ERR! stack     at ChildProcess._handle.onexit (node:internal/child_process:293:12)
npm error gyp ERR! System Windows_NT 10.0.22631
npm error gyp ERR! command "C:\\miniforge3\\envs\\bloom-desktop\\node.exe" "C:\\repos\\bloom-desktop-pilot\\app\\node_modules\\node-gyp\\bin\\node-gyp.js" "rebuild" "--release"
npm error gyp ERR! cwd C:\repos\bloom-desktop-pilot\app\node_modules\better-sqlite3
npm error gyp ERR! node -v v22.9.0
npm error gyp ERR! node-gyp -v v9.4.1
npm error gyp ERR! not ok
npm error A complete log of this run can be found in: C:\Users\Elizabeth\AppData\Local\npm-cache\_logs\2024-09-27T18_39_45_954Z-debug-0.log
  • nodejs=18.0 fixes the build issue for better-sqlite3
  • getting the following error when trying to open the app:
  File "C:/repos/bloom-desktop-pilot/pylon/pylon_stream_forever_fake.py", line 60, in <module>
    stream_frames(camera_settings)
  File "C:/repos/bloom-desktop-pilot/pylon/pylon_stream_forever_fake.py", line 27, in stream_frames
    src_frame = src_frames[i % len(src_frames)]
ZeroDivisionError: integer division or modulo by zero
Received line of length: 14
Received line of length: 2951656
data matches IMAGE
Received image: data:image/png;base6...
onCaptureImage() called with base64img.length:  2951650
stderr: DEBUG:PIL.PngImagePlugin:STREAM b'IHDR' 16 13
DEBUG:PIL.PngImagePlugin:STREAM b'bKGD' 41 2
DEBUG:PIL.PngImagePlugin:b'bKGD' 41 2 (unknown)
DEBUG:PIL.PngImagePlugin:STREAM b'zTXt' 55 58

stderr: DEBUG:PIL.PngImagePlugin:STREAM b'tEXt' 125 15
DEBUG:PIL.PngImagePlugin:STREAM b'IDAT' 152 8192
DEBUG:PIL.PngImagePlugin:STREAM b'IHDR' 16 13
DEBUG:PIL.PngImagePlugin:STREAM b'bKGD' 41 2
DEBUG:PIL.PngImagePlugin:b'bKGD' 41 2 (unknown)
DEBUG:PIL.PngImagePlugin:STREAM b'zTXt' 55 58
DEBUG:PIL.PngImagePlugin:STREAM b'tEXt' 125 15
DEBUG:PIL.PngImagePlugin:STREAM b'IDAT' 152 8192

stderr: DEBUG:PIL.Image:Error closing: Operation on closed image

Summary by CodeRabbit

Summary by CodeRabbit

  • Documentation
    • Updated setup instructions for the bloom-desktop environment to specify Python 3.8 and Node.js requirements.
    • Simplified installation steps by removing conda requirements.
    • Added a note about the necessity of C++ build tools from Visual Studio for setup.
    • Clarified steps for activating the conda environment and generating the .env file.
    • Specified required build tools for different operating systems and suggested using npm cache clean for cleanup.
    • Enhanced instructions for setting up a local Supabase instance and managing configuration files.

Copy link

coderabbitai bot commented Sep 27, 2024

Walkthrough

The changes involve updates to the README.md files for setting up the bloom-desktop environment and enhancing logging functionality in Python scripts. The installation instructions have been clarified by specifying the required versions of Python and Node.js, simplifying the setup steps, and adding a note about the necessity of C++ build tools. Additionally, logging has been introduced in the Python scripts to improve runtime tracking and error handling.

Changes

Files Change Summary
README.md, app/README.md Updated setup instructions to specify Python 3.8 and Node.js; clarified environment activation and installation steps; added note for C++ build tools.
pylon/pylon_fake.py, pylon/pylon_stream_forever_fake.py Introduced logging functionality; modified path handling with pathlib; added debug logs for various operations and error handling.

Poem

In the garden of code, where the bunnies hop,
New paths are laid, and the setup won't stop.
With Python and Node, the tools now align,
C++ build tools added, all will be fine!
So gather your carrots, let’s code with delight,
For bloom-desktop blooms, oh what a sight! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
app/README.md (1)

10-10: Consider formatting the URL as a proper Markdown link.

To improve readability and follow Markdown best practices, consider formatting the URL as a proper link.

Here's a suggested improvement:

-    https://visualstudio.microsoft.com/ C++ build tools required 
+    [C++ build tools](https://visualstudio.microsoft.com/) required

This change will make the link clickable in rendered Markdown and improve the overall appearance of the README.

🧰 Tools
🪛 Markdownlint

10-10: null
Bare URL used

(MD034, no-bare-urls)

README.md (2)

11-11: Approve change with suggestions for improvement

The updated instruction improves the setup process by explicitly specifying both Python and Node.js versions. However, there are a few points to consider:

  1. The spelling of "Node.js" should be corrected (currently "nodejs").
  2. Consider specifying Node.js version 18.0, as mentioned in the PR objectives to resolve the better-sqlite3 build issue.
  3. The PR objectives mention issues with nidaqmx and better-sqlite3. Should these be addressed in the README?

Here's a suggested revision:

- 1. Create the `bloom-desktop` environment with python 3.8 and nodejs: `mamba create --name bloom-desktop python=3.8 nodejs pip`
+ 1. Create the `bloom-desktop` environment with Python 3.8 and Node.js 18.0: `mamba create --name bloom-desktop python=3.8 nodejs=18.0 pip`

Additionally, consider adding a note about potential issues with nidaqmx and better-sqlite3, and how to resolve them if encountered during the setup process.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~11-~11: The official spelling of this programming framework is “Node.js”.
Context: ...esktopenvironment with python 3.8 and nodejs:mamba create --name bloom-desktop pyt...

(NODE_JS)


Line range hint 1-37: Suggest additional improvements to address PR objectives

While the changes improve the initial setup process, the README could be further enhanced to address the issues mentioned in the PR objectives:

  1. Add a note about the nidaqmx package installation issue:
    "Note: The nidaqmx package available from the conda channel may be broken. The pip version is up-to-date and recommended."

  2. Include information about potential better-sqlite3 compilation issues:
    "If you encounter compilation issues with better-sqlite3, ensure you're using Node.js version 18.0 or later."

  3. Consider adding a troubleshooting section to help users address common setup issues.

These additions will make the README more comprehensive and help users avoid or resolve the issues mentioned in the PR objectives.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~11-~11: The official spelling of this programming framework is “Node.js”.
Context: ...esktopenvironment with python 3.8 and nodejs:mamba create --name bloom-desktop pyt...

(NODE_JS)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 58fe37e and 5325732.

📒 Files selected for processing (2)
  • README.md (1 hunks)
  • app/README.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md

[uncategorized] ~11-~11: The official spelling of this programming framework is “Node.js”.
Context: ...esktopenvironment with python 3.8 and nodejs:mamba create --name bloom-desktop pyt...

(NODE_JS)

🪛 Markdownlint
app/README.md

10-10: null
Bare URL used

(MD034, no-bare-urls)

🔇 Additional comments (1)
app/README.md (1)

10-10: Excellent addition of C++ build tools requirement!

This new line provides crucial information for users setting up the Bloom Desktop application. It directly addresses the installation issues mentioned in the PR objectives, particularly the problems related to building native Node.js modules like better-sqlite3.

🧰 Tools
🪛 Markdownlint

10-10: null
Bare URL used

(MD034, no-bare-urls)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (5)
app/README.md (1)

11-17: Minor improvements for clarity and consistency

The additional details about build tools and the npm install process are helpful. Consider the following suggestions:

  1. Change "cleanup" to "clean up" for grammatical correctness.
  2. Convert the bare URL to a proper markdown link for better readability and to address the Markdownlint warning.

Here's a suggested update:

6. Run `npm install`.
    - Make sure you are in `app` directory. 
    - Build tools required for this step:
-         - https://visualstudio.microsoft.com/ C++ for Desktop Development on Windows
+         - [Visual Studio](https://visualstudio.microsoft.com/) C++ for Desktop Development on Windows
         - XCode for Mac
         - `build-essential` for Linux
-    - Use `npm cache clean` to cleanup
+    - Use `npm cache clean` to clean up
🧰 Tools
🪛 LanguageTool

[misspelling] ~17-~17: Did you mean the phrasal verb “clean up” instead of the noun ‘cleanup’?
Context: ...or Linux - Use npm cache clean to cleanup 7. Run npm run client:generate to gen...

(CLEAN_UP)

🪛 Markdownlint

14-14: null
Bare URL used

(MD034, no-bare-urls)

pylon/pylon_fake.py (2)

13-14: LGTM: Logging configuration added

The logging configuration is appropriately set to DEBUG level. This is useful for development and troubleshooting.

Consider adding a comment explaining why DEBUG level is used, or consider making the log level configurable through an environment variable for easier switching between production and development settings.


15-18: LGTM: Improved path handling for sample_scan

The changes to sample_scan are well-implemented:

  1. Using pathlib for path construction is a good practice.
  2. The relative path makes the code more portable across different environments.
  3. The debug log statement aids in troubleshooting.

Consider removing the commented-out line (15) as it's no longer needed and can be retrieved from version control if necessary.

pylon/pylon_stream_forever_fake.py (2)

Line range hint 9-19: Approve logging implementation with a suggestion for production use.

The addition of logging is a great improvement for debugging and monitoring. However, consider adjusting the logging level for production use.

For production, consider using a less verbose logging level:

import logging

logging.basicConfig(level=logging.INFO)  # or logging.WARNING for production

This will reduce the amount of log output in production while still capturing important information.

🧰 Tools
🪛 Ruff

28-28: Local variable n is assigned to but never used

Remove assignment to unused variable n

(F841)


53-63: Approve error handling and logging improvements with a suggestion.

The additions to the img_to_base64 function significantly improve its robustness and debuggability:

  • Error handling for invalid images is a great addition.
  • The use of context managers for BytesIO and PIL Image ensures proper resource management.
  • The debug logs provide valuable information about the image processing steps.

Consider modifying the base64 logging to avoid potential exposure of sensitive data:

logging.debug(f"base64_img length: {len(base64_img)}")

This change will still provide useful information without risking the exposure of image data in logs.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 5325732 and 62319e6.

📒 Files selected for processing (4)
  • README.md (1 hunks)
  • app/README.md (1 hunks)
  • pylon/pylon_fake.py (1 hunks)
  • pylon/pylon_stream_forever_fake.py (4 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md

[uncategorized] ~11-~11: The official spelling of this programming framework is “Node.js”.
Context: ...esktopenvironment with python 3.8 and nodejs:mamba create --name bloom-desktop pyt...

(NODE_JS)

app/README.md

[misspelling] ~17-~17: Did you mean the phrasal verb “clean up” instead of the noun ‘cleanup’?
Context: ...or Linux - Use npm cache clean to cleanup 7. Run npm run client:generate to gen...

(CLEAN_UP)

🪛 Markdownlint
app/README.md

14-14: null
Bare URL used

(MD034, no-bare-urls)

🪛 Ruff
pylon/pylon_stream_forever_fake.py

28-28: Local variable n is assigned to but never used

Remove assignment to unused variable n

(F841)

🔇 Additional comments (14)
README.md (3)

Line range hint 1-33: Overall README improvements

The changes to the README file generally improve the setup process by providing more specific instructions and simplifying the installation steps. However, there are a few areas where additional clarity could be beneficial:

  1. Explicitly mentioning the change from conda to pip for nidaqmx installation.
  2. Providing context for why these changes were made.
  3. Ensuring that the unchanged sections (NI-DAQ Control and Basler Pylon) are still accurate and complete.

These improvements will help future contributors better understand the setup process and the reasoning behind certain decisions.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~11-~11: The official spelling of this programming framework is “Node.js”.
Context: ...esktopenvironment with python 3.8 and nodejs:mamba create --name bloom-desktop pyt...

(NODE_JS)


11-11: 🛠️ Refactor suggestion

Improve environment setup instruction

The updated instruction provides more specific versions for Python and Node.js, which is excellent for reproducibility. However, there are a few points to consider:

  1. The official spelling of Node.js should be used instead of "nodejs".
  2. The PR objectives mention issues with nidaqmx installation from conda, but this change isn't reflected in the README.

Consider updating the instruction as follows:

-1. Create the `bloom-desktop` environment with python 3.8 and nodejs: `mamba create --name bloom-desktop python=3.8 nodejs=18 pip -c conda-forge`
+1. Create the `bloom-desktop` environment with Python 3.8 and Node.js 18: `mamba create --name bloom-desktop python=3.8 nodejs=18 pip -c conda-forge`

Also, consider adding a note about the nidaqmx installation issue and why it's now installed via pip instead of conda.

To ensure consistency with the project's actual requirements, let's check the requirements.txt file:

#!/bin/bash
# Description: Verify the contents of requirements.txt
cat requirements.txt
🧰 Tools
🪛 LanguageTool

[uncategorized] ~11-~11: The official spelling of this programming framework is “Node.js”.
Context: ...esktopenvironment with python 3.8 and nodejs:mamba create --name bloom-desktop pyt...

(NODE_JS)


Line range hint 16-33: Review NI-DAQ and Basler Pylon setup instructions

While these sections weren't changed in this PR, consider reviewing them to ensure they're still accurate and complete, especially given the issues mentioned in the PR objectives (ZeroDivisionError and problems with closing captured images).

It might be helpful to verify if these instructions are still valid and if any additional steps are needed to address the reported issues. Consider running the following commands to check for any recent changes or issues related to these components:

✅ Verification successful

Verification Successful: README.md Setup Instructions Are Current

The setup instructions for NI-DAQ Control and Basler Pylon in README.md remain accurate and have not been affected by recent changes.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for recent changes or issues related to NI-DAQ and Basler Pylon
git log -n 5 -- "*daq*" "*pylon*"
git grep -n "ZeroDivisionError"
git grep -n "onCaptureImage"

Length of output: 1419

🧰 Tools
🪛 LanguageTool

[uncategorized] ~11-~11: The official spelling of this programming framework is “Node.js”.
Context: ...esktopenvironment with python 3.8 and nodejs:mamba create --name bloom-desktop pyt...

(NODE_JS)

app/README.md (5)

5-10: LGTM: Clear and detailed installation steps

The updated installation instructions provide a clear, step-by-step process for setting up the Bloom Desktop application. The addition of the conda environment activation step and the detailed instructions for creating and editing configuration files are helpful improvements.


18-19: LGTM: Clear instructions for client generation and database deployment

The added steps for generating the TypeScript client and deploying the database are clear and provide necessary information for setting up the application.


23-23: LGTM: Clear Quickstart instruction

The Quickstart instruction is clear and concise, providing users with a simple command to run the application from the correct directory.


Line range hint 33-39: LGTM: Comprehensive software update instructions

The "Updating the software" section provides clear and comprehensive instructions for users to update their existing installation. The steps cover all necessary aspects of the update process, including pulling changes, updating dependencies, regenerating the client, and applying database migrations.

🧰 Tools
🪛 LanguageTool

[misspelling] ~17-~17: Did you mean the phrasal verb “clean up” instead of the noun ‘cleanup’?
Context: ...or Linux - Use npm cache clean to cleanup 7. Run npm run client:generate to gen...

(CLEAN_UP)

🪛 Markdownlint

14-14: null
Bare URL used

(MD034, no-bare-urls)


Line range hint 1-39: Great improvements to the README file

The changes to the README file significantly enhance its clarity and usefulness. The installation instructions are now more detailed and comprehensive, covering environment setup, configuration, and build requirements. The added Quickstart section and the clear update instructions make it easier for users to get started and maintain the Bloom Desktop application.

These improvements will greatly assist users in setting up and using the application, potentially reducing installation-related issues and support requests.

🧰 Tools
🪛 LanguageTool

[misspelling] ~17-~17: Did you mean the phrasal verb “clean up” instead of the noun ‘cleanup’?
Context: ...or Linux - Use npm cache clean to cleanup 7. Run npm run client:generate to gen...

(CLEAN_UP)

🪛 Markdownlint

14-14: null
Bare URL used

(MD034, no-bare-urls)

pylon/pylon_fake.py (2)

9-9: LGTM: Logging import added

The addition of the logging import is appropriate for the new logging functionality introduced in this file.


Line range hint 1-18: Summary: Improvements in logging and path handling

The changes in this file enhance its functionality and maintainability:

  1. Addition of logging improves debugging capabilities.
  2. The use of pathlib for sample_scan increases portability across different environments.
  3. The changes align well with the PR objectives, addressing issues related to path handling.

These modifications are well-implemented and improve the overall quality of the code.

pylon/pylon_stream_forever_fake.py (4)

20-24: Approve path handling improvements.

The use of pathlib for handling the sample scan path is an excellent improvement. It enhances cross-platform compatibility and makes the code more robust. The added debug log is helpful for confirming the correct path resolution.


30-30: Approve addition of frame count logging.

The debug log for reporting the number of frames found is a helpful addition. It provides valuable information for debugging and monitoring the stream_frames function.


40-40: Approve addition of image read logging.

The debug log for confirming when an image is read is a valuable addition. It enhances the ability to track the image reading process during execution.


Line range hint 1-83: Overall improvements in logging and error handling.

The changes made to this file significantly enhance its debuggability and robustness:

  1. Addition of comprehensive logging throughout the script.
  2. Improved error handling in the img_to_base64 function.
  3. Better path handling using pathlib.

These improvements align well with the PR objectives, particularly in addressing installation and runtime issues. The enhanced logging will be valuable for diagnosing problems like the ZeroDivisionError mentioned in the PR summary.

To further improve the code:

  1. Consider adjusting the logging level for production use.
  2. Implement the functionality for the num_frames setting or remove it if unused.
  3. Be cautious about logging potentially sensitive data in the base64 string.

These changes provide a solid foundation for better error detection and debugging in the bloom-desktop-pilot project.

🧰 Tools
🪛 Ruff

28-28: Local variable n is assigned to but never used

Remove assignment to unused variable n

(F841)

Comment on lines 13 to 14
3. Install pip requirements:
`pip install -r requirements.txt`
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Clarify the change in installation method

The updated instruction simplifies the setup process by focusing solely on pip requirements, which aligns with the PR objectives regarding nidaqmx installation issues. However, it might be beneficial to provide more context for this change.

Consider adding a brief explanation:

 3. Install pip requirements:
    `pip install -r requirements.txt`
+   Note: We now use pip to install all requirements, including `nidaqmx`, due to issues with the conda version.

This addition would help future contributors understand the reasoning behind the installation method change.

Committable suggestion was skipped due to low confidence.

# Test images are in "test/sample_scan" directory from the root of the repo
sample_scan = pathlib.Path(__file__).parent.parent / "test" / "sample_scan"
logging.debug(f"sample_scan: {sample_scan}")



def stream_frames(camera_settings):
n = camera_settings["num_frames"]
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove unused variable 'n'.

The local variable 'n' is assigned but never used in the function. This was detected by the static analysis tool.

Consider removing the unused variable:

def stream_frames(camera_settings):
-    n = camera_settings["num_frames"]
    src_frames = glob.glob(os.path.join(sample_scan, "*.png"))

If the num_frames setting is intended to be used, please implement its functionality or remove it from the camera_settings if it's no longer needed.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
n = camera_settings["num_frames"]
def stream_frames(camera_settings):
src_frames = glob.glob(os.path.join(sample_scan, "*.png"))
🧰 Tools
🪛 Ruff

28-28: Local variable n is assigned to but never used

Remove assignment to unused variable n

(F841)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (6)
app/README.md (3)

7-12: Approve changes with a suggestion for clarity.

The additional information about test images and environment variables is helpful. However, the reference to the external environment variables could be more explicit.

Consider adding a brief explanation of why these environment variables are needed. For example:

- Use the same environment variables as in https://gitlab.com/salk-tm/bloom/-/blob/main/web/env.dev to configure bloom-desktop to work with your local supabase instance.
+ Use the same environment variables as in https://gitlab.com/salk-tm/bloom/-/blob/main/web/env.dev to configure bloom-desktop to work with your local Supabase instance. These variables include necessary credentials and configuration settings for the Supabase connection.
🧰 Tools
🪛 Markdownlint

11-11: null
Bare URL used

(MD034, no-bare-urls)


13-19: Approve changes with a suggestion for clarity on cache cleaning.

The additional details about build tools for different operating systems are very helpful. However, the instruction for cache cleaning can be improved.

Consider rewording the cache cleaning instruction for clarity:

-    - Use `npm cache clean` to cleanup
+    - If you encounter issues, try cleaning the npm cache with `npm cache clean --force`

This change provides more context on when to use the cache cleaning command and uses the correct phrasal verb "clean up".

🧰 Tools
🪛 LanguageTool

[misspelling] ~19-~19: Did you mean the phrasal verb “clean up” instead of the noun ‘cleanup’?
Context: ...or Linux - Use npm cache clean to cleanup 8. Run npm run client:generate to gen...

(CLEAN_UP)

🪛 Markdownlint

16-16: null
Bare URL used

(MD034, no-bare-urls)


Line range hint 27-46: Consider reviewing unchanged sections for consistency.

While the "Modifying the database" and "Updating the software" sections haven't been changed, it might be beneficial to review them to ensure they align with the updated installation instructions. For example, check if any new steps or considerations from the installation process should be reflected in the update process.

🧰 Tools
🪛 LanguageTool

[grammar] ~6-~6: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...tm/bloom/-/tree/main?ref_type=heads] to setup a local supabase instance to interact w...

(NOUN_VERB_CONFUSION)


[misspelling] ~19-~19: Did you mean the phrasal verb “clean up” instead of the noun ‘cleanup’?
Context: ...or Linux - Use npm cache clean to cleanup 8. Run npm run client:generate to gen...

(CLEAN_UP)

🪛 Markdownlint

6-6: (bloom)[https://gitlab.com/salk-tm/bloom/-/tree/main?ref_type=heads]
Reversed link syntax

(MD011, no-reversed-links)


11-11: null
Bare URL used

(MD034, no-bare-urls)


16-16: null
Bare URL used

(MD034, no-bare-urls)

pylon/pylon_stream_forever_fake.py (3)

20-22: LGTM: Improved path handling with pathlib.

The use of pathlib for handling the sample scan path is a good improvement. It provides a more robust and cross-platform way to manage file paths.

Consider using an f-string for the logging statement to make it more concise:

-logging.info(f"sample_scan: {sample_scan}")
+logging.info(f"{sample_scan=}")

This change uses the new Python 3.8+ feature of self-documenting expressions in f-strings, which includes both the variable name and its value in the output.


52-62: LGTM: Improved error handling and resource management in img_to_base64.

The changes in the img_to_base64 function are well-implemented:

  • Error handling for invalid images has been added.
  • Context managers are now used for BytesIO and PIL Image, ensuring proper resource management.
  • Debug logging has been added at various stages, which will aid in troubleshooting.

These improvements enhance the function's robustness and observability.

Consider using a more descriptive error message when logging invalid images:

-        logging.error("Invalid image encountered!")
+        logging.error(f"Invalid image encountered: size={img.size if img is not None else None}")

This change provides more context about why the image is considered invalid, which could be helpful for debugging.


Line range hint 1-80: Overall improvements and next steps.

The changes made to this file significantly improve its robustness and observability:

  1. Comprehensive logging has been added throughout the script.
  2. Error handling has been enhanced, particularly in the img_to_base64 function.
  3. The use of pathlib for path handling improves cross-platform compatibility.

These improvements align well with the PR objectives of addressing runtime issues, particularly the ZeroDivisionError mentioned. The added logging will help diagnose such issues more effectively.

While these changes are valuable, they don't directly address the installation issues with nidaqmx and better-sqlite3 mentioned in the PR objectives. Consider the following next steps:

  1. Document the resolution of using nodejs=18.0 for the better-sqlite3 build issue in the project's README or installation guide.
  2. If not already done, update the project's dependency specifications to reflect the correct versions that work together (e.g., specifying nodejs=18.0 in the environment setup).
  3. Consider adding error handling around the parts of the code that previously caused the ZeroDivisionError to provide more informative error messages.

These steps will help ensure that the installation process is smooth for other developers and that the runtime behavior is more predictable and easier to debug.

🧰 Tools
🪛 Ruff

27-27: Local variable n is assigned to but never used

Remove assignment to unused variable n

(F841)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 62319e6 and bea3e58.

📒 Files selected for processing (3)
  • app/README.md (1 hunks)
  • pylon/pylon_fake.py (1 hunks)
  • pylon/pylon_stream_forever_fake.py (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • pylon/pylon_fake.py
🧰 Additional context used
🪛 LanguageTool
app/README.md

[grammar] ~6-~6: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...tm/bloom/-/tree/main?ref_type=heads] to setup a local supabase instance to interact w...

(NOUN_VERB_CONFUSION)


[misspelling] ~19-~19: Did you mean the phrasal verb “clean up” instead of the noun ‘cleanup’?
Context: ...or Linux - Use npm cache clean to cleanup 8. Run npm run client:generate to gen...

(CLEAN_UP)

🪛 Markdownlint
app/README.md

6-6: (bloom)[https://gitlab.com/salk-tm/bloom/-/tree/main?ref_type=heads]
Reversed link syntax

(MD011, no-reversed-links)


11-11: null
Bare URL used

(MD034, no-bare-urls)


16-16: null
Bare URL used

(MD034, no-bare-urls)

🪛 Ruff
pylon/pylon_stream_forever_fake.py

27-27: Local variable n is assigned to but never used

Remove assignment to unused variable n

(F841)

🔇 Additional comments (5)
app/README.md (2)

20-21: LGTM: Clear instructions for client generation and database deployment.

These steps provide clear instructions for generating the TypeScript client and deploying the database. The explicit mention of the schema file location is helpful.


25-25: LGTM: Helpful directory specification for start command.

The addition of the directory information for running the start command is a useful clarification for users.

pylon/pylon_stream_forever_fake.py (3)

9-9: LGTM: Logging configuration added.

The addition of logging functionality is a good practice. It will help with debugging and monitoring the application's behavior.

Also applies to: 18-18


29-29: LGTM: Enhanced logging in stream_frames.

The addition of debug log statements in the stream_frames function improves observability. This will be helpful for troubleshooting and understanding the function's behavior during execution.

Also applies to: 39-39


27-27: ⚠️ Potential issue

Remove unused variable 'n'.

The local variable 'n' is assigned but never used in the function. This was detected by the static analysis tool.

Consider removing the unused variable:

def stream_frames(camera_settings):
-    n = camera_settings["num_frames"]
    src_frames = glob.glob(os.path.join(sample_scan, "*.png"))

If the num_frames setting is intended to be used, please implement its functionality or remove it from the camera_settings if it's no longer needed.

🧰 Tools
🪛 Ruff

27-27: Local variable n is assigned to but never used

Remove assignment to unused variable n

(F841)

Comment on lines +5 to +6
1. `mamba activate bloom-desktop`
2. Follow the instructions in (bloom)[https://gitlab.com/salk-tm/bloom/-/tree/main?ref_type=heads] to setup a local supabase instance to interact with, and add some test data.
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix link formatting and grammar in Supabase setup instruction.

The new steps are helpful, but there are two issues to address:

  1. The link to the Bloom repository is formatted incorrectly. Use square brackets for the text and parentheses for the URL.
  2. The word "setup" should be "set up" when used as a verb.

Please apply the following changes:

-2. Follow the instructions in (bloom)[https://gitlab.com/salk-tm/bloom/-/tree/main?ref_type=heads] to setup a local supabase instance to interact with, and add some test data.
+2. Follow the instructions in [bloom](https://gitlab.com/salk-tm/bloom/-/tree/main?ref_type=heads) to set up a local Supabase instance to interact with, and add some test data.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
1. `mamba activate bloom-desktop`
2. Follow the instructions in (bloom)[https://gitlab.com/salk-tm/bloom/-/tree/main?ref_type=heads] to setup a local supabase instance to interact with, and add some test data.
1. `mamba activate bloom-desktop`
2. Follow the instructions in [bloom](https://gitlab.com/salk-tm/bloom/-/tree/main?ref_type=heads) to set up a local Supabase instance to interact with, and add some test data.
🧰 Tools
🪛 LanguageTool

[grammar] ~6-~6: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...tm/bloom/-/tree/main?ref_type=heads] to setup a local supabase instance to interact w...

(NOUN_VERB_CONFUSION)

🪛 Markdownlint

6-6: (bloom)[https://gitlab.com/salk-tm/bloom/-/tree/main?ref_type=heads]
Reversed link syntax

(MD011, no-reversed-links)

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.

1 participant