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

Fixing MNIST dataloader #133

Merged
merged 6 commits into from Dec 3, 2021
Merged

Fixing MNIST dataloader #133

merged 6 commits into from Dec 3, 2021

Conversation

tihbe
Copy link
Contributor

@tihbe tihbe commented Nov 30, 2021

tutorials/end_to_end/tutorial01_mnist_digit_classification.ipynb doesn't work if you install lava with a pip install as mnist.npy is not packaged. I fixed it by completing these to-dos.

@mgkwill
Copy link
Contributor

mgkwill commented Nov 30, 2021

@tihbe Thanks for the pull request!

We generally require an issue for each pull request describing the intent of the PR linked to the PR.

Beside that, it looks like you have a bandit security linting issue. Please run bandit -r src/lava/ --format custom --msg-template and fix the issue in question. I've assigned some reviewers.

@tihbe
Copy link
Contributor Author

tihbe commented Nov 30, 2021

@mgkwill Thanks for the info ! Did you want me to create an issue for this PR first ?
I'm not 100% sure how to fix the security linter issue. It seems that using urllib.request.urlopen automatically triggers the linter. I've disabled the specific line triggering the error as suggested here.

src/lava/utils/dataloader/mnist.py Outdated Show resolved Hide resolved
src/lava/utils/dataloader/mnist.py Outdated Show resolved Hide resolved
@mgkwill
Copy link
Contributor

mgkwill commented Nov 30, 2021

Thanks for the updates @tihbe and trying to sort out the bandit security linting issues.

@awintel awintel added this to Less import | Urgent in lava Dec 3, 2021
@srrisbud
Copy link
Contributor

srrisbud commented Dec 3, 2021

@tihbe, have you addressed the change requested by @mgkwill?

@tihbe tihbe requested a review from mgkwill December 3, 2021 09:19
@tihbe
Copy link
Contributor Author

tihbe commented Dec 3, 2021

@srrisbud Yes

@mgkwill mgkwill merged commit 1c83f1f into lava-nc:main Dec 3, 2021
lava automation moved this from Less import | Urgent to Done Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
lava
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants