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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
