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

update anomaly detection app dependencies #672

Merged

Conversation

johnsonshih
Copy link
Contributor

What this PR does / why we need it:
Add missing dependency to anomaly detection app, the python3-sklearn was removed accidentally in previous PR, the dependency is required or the application cannot run properly.

Also remove the version stamp for numpy and let the system picks proper version to use. Use numpy version 1.21.4 causes issues when building arm32v7 images. The build stuck at installing dependencies and never complete.

Special notes for your reviewer:

If applicable:

  • this PR has an associated PR with documentation in akri-docs
  • this PR contains unit tests
  • added code adheres to standard Rust formatting (cargo fmt)
  • code builds properly (cargo build)
  • code is free of common mistakes (cargo clippy)
  • all Akri tests succeed (cargo test)
  • inline documentation builds (cargo doc)
  • all commits pass the DCO bot check by being signed off -- see the failing DCO check for instructions on how to retroactively sign commits

Signed-off-by: Johnson Shih <[email protected]>
Copy link
Contributor

@diconico07 diconico07 left a comment

Choose a reason for hiding this comment

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

If I understand correctly, this is needed to make armv7 version to work, if so, as it's just a sample app I guess we can disable armv7 build for this one.

@johnsonshih
Copy link
Contributor Author

johnsonshih commented Oct 17, 2023

If I understand correctly, this is needed to make armv7 version to work, if so, as it's just a sample app I guess we can disable armv7 build for this one.

yes, but I'd like to fix the build first, we can disable armv7 build in a separate PR so in case we bring armv7 back in the future, it still works.
BTW, currently the dev build is broken, we only have v0.12.10-dev released, we need to unblock the build to so users can install helm chart using the latest version tag.

Copy link
Contributor

@diconico07 diconico07 left a comment

Choose a reason for hiding this comment

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

Fine for me, I still think the best way for a fix would be to just disable armv7 here.
As the only way to get back an armv7 build (if we switch to python base image rather than debian + apt python) for this sample app would be to not depend on numpy and scikit (maybe we could use a simpler way than LOF for this, I'm not sure of the value of something that complex for a sample app).

But we can postpone that discussion and just fix the build for now

@johnsonshih
Copy link
Contributor Author

Fine for me, I still think the best way for a fix would be to just disable armv7 here. As the only way to get back an armv7 build (if we switch to python base image rather than debian + apt python) for this sample app would be to not depend on numpy and scikit (maybe we could use a simpler way than LOF for this, I'm not sure of the value of something that complex for a sample app).

But we can postpone that discussion and just fix the build for now

agree

@johnsonshih johnsonshih merged commit 172eb2b into project-akri:main Oct 19, 2023
40 checks passed
@johnsonshih johnsonshih deleted the user/jshih/anomaly-detection-app branch October 19, 2023 14:28
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