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

Config: Allow bypassing low memory suggestion #1611

Closed
wants to merge 1 commit into from

Conversation

petertrr
Copy link

@petertrr petertrr commented Oct 8, 2021

After one of the recent releases I found out that tensorflow is disabled on my device (with 1.7 GiB of memory as reported by free). I understand the concern, but I wanted to try to run tensorflow anyway and see how bad it is. So I added a way to bypass this check with an environment variable. I named it unsafe_ to emphasize that it's not stable.

It turned out that my device still can run image recognition with these resources, so I would really like to have a way to disable low memory check. I'm not very familiar with Go, so if my change is not properly implemented or can be enhanced, I'll be happy to do it.

Another note, when tensorflow is disabled because of low mem, a checkbox 'Disable tensorflow' on frontend is checked and cannot be unchecked.

@CLAassistant
Copy link

CLAassistant commented Oct 8, 2021

CLA assistant check
All committers have signed the CLA.

@lastzero
Copy link
Member

lastzero commented Oct 8, 2021

What threshold would you be comfortable with? We received a ton of "bug" reports by people who didn't have enough memory and no swap configured.

@petertrr
Copy link
Author

petertrr commented Oct 8, 2021

What threshold would you be comfortable with? We received a ton of "bug" reports by people who didn't have enough memory and no swap configured.

I understand your concern, so I suggested not to simply reduce the threshold, but to allow user to explicitly disable it. I think that current default value of 2 GiB is reasonable, but if I want to experiment with my setup, I now have no other option other that to reduce or turn off the threshold in code and recompile the entire app.

@lastzero lastzero self-assigned this Oct 9, 2021
@lastzero lastzero added please-test Ready for acceptance test idea Feedback wanted / feature request labels Oct 9, 2021
@petertrr
Copy link
Author

I'll close this PR as I see the feature is implemented in 09f50fc

@petertrr petertrr closed this Oct 10, 2021
@lastzero
Copy link
Member

Yes, I was just going to ask if you're happy with this? I've changed the variable name to PHOTOPRISM_UNSAFE as we've decided to prefix all our variables with PHOTOPRISM_. Also you didn't sign the CLA yet, so I wasn't allowed to merge the PR directly.

@petertrr
Copy link
Author

Yes, I was just going to ask if you're happy with this? I've changed the variable name to PHOTOPRISM_UNSAFE as we've decided to prefix all our variables with PHOTOPRISM_. Also you didn't sign the CLA yet, so I wasn't allowed to merge the PR directly.

Yes, no problem at all. Thanks!

@graciousgrey graciousgrey added released Available in the stable release and removed please-test Ready for acceptance test labels Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea Feedback wanted / feature request released Available in the stable release
Projects
Status: Release 🌈
Development

Successfully merging this pull request may close these issues.

None yet

4 participants