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

QR code workflow step #286

Merged
merged 3 commits into from
Feb 26, 2024
Merged

QR code workflow step #286

merged 3 commits into from
Feb 26, 2024

Conversation

sberan
Copy link
Contributor

@sberan sberan commented Feb 26, 2024

Description

This is a QR code step. @PawelPeczek-Roboflow would you be willing to give some preliminary feedback on the approach? I want to make sure I'm doing things the right way since it's my first workflow step contribution.

HERE is a Loom as well as a script that runs QR detections.

Once the approach is vetted I'll update docs.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

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

Unit tests

Any specific deployment considerations

For example, documentation changes, usability, usage/costs, secrets, etc.

Docs

  • Docs updated? What were the changes:

@sberan sberan marked this pull request as draft February 26, 2024 03:34
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.

Well, this is exactly how that should look like

I am thinking of improving the internal design of compiler/executor - do u have any suggestions?

@PawelPeczek-Roboflow
Copy link
Collaborator

I feel like all it takes to push it would be making flake happy

@sberan
Copy link
Contributor Author

sberan commented Feb 26, 2024

do u have any suggestions?

  • it was very hard to figure out how to unit test. I think my ideal test setup would be more of an "integration test" style where a full workflow is passed in and we can make assertions about the result.
  • Setting the parent_id and reading images was kind of confusing. Especially that the parameter is called image but it has multiple images. It would be great to have an abstract class that would handle:
    • setting the parent_id
    • reading images
    • returning parent_id and image_metadata

What do you think of these suggestions? Should I put them in this PR?

@PawelPeczek-Roboflow
Copy link
Collaborator

Full agreement in terms of testing - in fact I do have some amount of workflows in my notebooks that I run to test e2e, that should be moved into integration tests. What I am usually testing unit-wise is the behaviour of step entity (mainly validation) and basic things related to step execution logic.

On the latter - great points - those conceptions are truly not so great in terms of clarity - emerged as we go further in development (for instance to make it compatible with supervision) - I think we should make changes in internal design, although I would not say you should implement those changes now.

@sberan
Copy link
Contributor Author

sberan commented Feb 26, 2024

I think we should make changes in internal design, although I would not say you should implement those changes now.

Sounds good! Are you able to make changes to the code and then test out in a notebook? I found that rebuilding the docker container took about 90 seconds each time I made a code change, which is why I did most of my development against the unit tests.

@PawelPeczek-Roboflow
Copy link
Collaborator

Well, I am a little bit sneaky with that, as I am using this in my notebook

import sys
sys.path.append("<LOCALTION_OF_INFERENCE_REPO>")

.... # the code to execute workflow

and all it takes to have a new changes is to restart kernel and run cells

@sberan sberan changed the title [DRAFT] qr code step QR code workflow step Feb 26, 2024
@sberan sberan marked this pull request as ready for review February 26, 2024 18:00
@sberan
Copy link
Contributor Author

sberan commented Feb 26, 2024

@PawelPeczek-Roboflow I added docs and fixed the build, approve if it looks good!

@PawelPeczek-Roboflow
Copy link
Collaborator

great!

@sberan sberan merged commit a6acb3f into main Feb 26, 2024
5 checks passed
@sberan sberan deleted the qr-step branch February 26, 2024 19:02
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