You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@singa.apache.org by "eyumboo (via GitHub)" <gi...@apache.org> on 2026/07/02 03:36:43 UTC
[PR] Add the implementation for downloading mnist [singa]
eyumboo opened a new pull request, #1383:
URL: https://github.com/apache/singa/pull/1383
Add the implementation fThis PR adds download_mnist.py to examples/singa_peft/examples/data.or downloading mnist
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@singa.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add the implementation for downloading mnist [singa]
Posted by "moazreyad (via GitHub)" <gi...@apache.org>.
moazreyad commented on code in PR #1383:
URL: https://github.com/apache/singa/pull/1383#discussion_r3522919345
##########
examples/singa_peft/examples/data/download_mnist.py:
##########
@@ -0,0 +1,49 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+import os
+import urllib.request
+
+
+def check_exist_or_download(url):
+
+ download_dir = '/tmp/' # downloaded to the /tmp/ folder
+ name = url.rsplit('/', 1)[-1]
+ filename = os.path.join(download_dir, name)
+
+ if not os.path.isfile(filename):
+ print("Downloading %s" % url)
+ urllib.request.urlretrieve(url, filename)
+ else:
+ print("Already Downloaded: %s" % url)
Review Comment:
### Suggestion: improve download logging clarity
The current download log is helpful, but it would be even better if we also print the resolved local path (or target directory) where files are stored. This makes it easier to debug storage issues and verify where artifacts end up, especially since the download location may depend on defaults or environment variables (e.g. `/tmp/mnist`).
In addition, it would improve clarity to include the local filename/path in both download states.
#### Proposed changes:
```python
if not os.path.isfile(filename):
print("Downloading %s -> %s" % (url, filename))
urllib.request.urlretrieve(url, filename)
else:
print("Already Downloaded: %s -> %s" % (url, filename))
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@singa.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add the implementation for downloading mnist [singa]
Posted by "eyumboo (via GitHub)" <gi...@apache.org>.
eyumboo commented on PR #1383:
URL: https://github.com/apache/singa/pull/1383#issuecomment-4861964633
Add the implementation for downloading mnist
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@singa.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add the implementation for downloading mnist [singa]
Posted by "eyumboo (via GitHub)" <gi...@apache.org>.
eyumboo closed pull request #1383: Add the implementation for downloading mnist
URL: https://github.com/apache/singa/pull/1383
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@singa.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add the implementation for downloading mnist [singa]
Posted by "eyumboo (via GitHub)" <gi...@apache.org>.
eyumboo commented on PR #1383:
URL: https://github.com/apache/singa/pull/1383#issuecomment-4861962286
Add the implementation for downloading mnist
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@singa.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add the implementation for downloading mnist [singa]
Posted by "moazreyad (via GitHub)" <gi...@apache.org>.
moazreyad commented on PR #1383:
URL: https://github.com/apache/singa/pull/1383#issuecomment-4882416834
One remaining integration suggestion for a future PR: `examples/singa_peft/examples/autograd/mnist_cnn.py` currently [expects](https://github.com/eyumboo/singa/blob/dev-postgresql/examples/singa_peft/examples/autograd/mnist_cnn.py#L72) the MNIST files to be located under `/tmp`, while the downloader now defaults to `/tmp/mnist`.
As a result, running the downloader with its default settings is not sufficient for training to work out of the box.
A future improvement could be to either:
* update `mnist_cnn.py` to use `/tmp/mnist` (or the same default as the downloader), or
* document that users should invoke the downloader with `--dir-path /tmp` when using `mnist_cnn.py`.
Ideally, both scripts should share the same default directory to avoid this mismatch.
Another non-blocking suggestion for a future cleanup is the directory structure under `examples/singa_peft/examples`, which feels a bit redundant since everything under `examples/` is already example code. It might be worth reorganizing this in a follow-up PR by removing the second `examples` directory and simplifying the layout. A flatter, more consistent structure would make the project easier to navigate and reduce unnecessary nesting.
Neither of these is a blocking issue for merging this PR, but addressing them in a follow-up PR would improve the out-of-the-box experience, project organization, and overall clarity for future users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@singa.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add the implementation for downloading mnist [singa]
Posted by "moazreyad (via GitHub)" <gi...@apache.org>.
moazreyad commented on code in PR #1383:
URL: https://github.com/apache/singa/pull/1383#discussion_r3520492807
##########
examples/singa_peft/examples/data/download_mnist.py:
##########
@@ -0,0 +1,49 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+import os
+import urllib.request
+
+
+def check_exist_or_download(url):
+
+ download_dir = '/tmp/' # downloaded to the /tmp/ folder
+ name = url.rsplit('/', 1)[-1]
+ filename = os.path.join(download_dir, name)
+
+ if not os.path.isfile(filename):
+ print("Downloading %s" % url)
+ urllib.request.urlretrieve(url, filename)
+ else:
+ print("Already Downloaded: %s" % url)
+
+
+if __name__ == '__main__':
+
+ # List urls of the mnist dataset
+ train_x_url = 'http://yann.lecun.com/exdb/mnist/train-images-idx3-ubyte.gz'
+ train_y_url = 'http://yann.lecun.com/exdb/mnist/train-labels-idx1-ubyte.gz'
+ valid_x_url = 'http://yann.lecun.com/exdb/mnist/t10k-images-idx3-ubyte.gz'
+ valid_y_url = 'http://yann.lecun.com/exdb/mnist/t10k-labels-idx1-ubyte.gz'
Review Comment:
These four URLs appear to be invalid now. Could you please check them and update them if necessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@singa.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add the implementation for downloading mnist [singa]
Posted by "moazreyad (via GitHub)" <gi...@apache.org>.
moazreyad commented on code in PR #1383:
URL: https://github.com/apache/singa/pull/1383#discussion_r3522906163
##########
examples/singa_peft/examples/data/download_mnist.py:
##########
@@ -0,0 +1,49 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+import os
+import urllib.request
+
+
+def check_exist_or_download(url):
+
+ download_dir = '/tmp/' # downloaded to the /tmp/ folder
Review Comment:
`download_dir` is currently hard-coded to `/tmp/`, but the PEFT MNIST loader defaults to `/tmp/mnist` and expects the dataset files under that directory:
- Default dataset path: https://github.com/eyumboo/singa/blob/dev-postgresql/examples/singa_peft/examples/data/mnist.py#L85
- File lookup logic assumes `/tmp/mnist`: https://github.com/eyumboo/singa/blob/dev-postgresql/examples/singa_peft/examples/data/mnist.py#L38
In addition, the example runner explicitly passes `-dir /tmp/mnist`:
https://github.com/eyumboo/singa/blob/dev-postgresql/examples/singa_peft/examples/run.sh#L21
Because of this mismatch, even after running the new downloader, training fails since it looks for files like `/tmp/mnist/train-images-idx3-ubyte.gz`, which are not present.
It would be better if the downloader:
- Writes directly to `/tmp/mnist` (and creates the directory if it doesn’t exist)
- Ideally supports the same `--dir-path` argument used by the training pipeline for consistency
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@singa.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add the implementation for downloading mnist [singa]
Posted by "moazreyad (via GitHub)" <gi...@apache.org>.
moazreyad merged PR #1383:
URL: https://github.com/apache/singa/pull/1383
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@singa.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org