wangdatan commented on issue #132: SUBMARINE-296. [SDK] Submarine pipeline example URL: https://github.com/apache/submarine/pull/132#issuecomment-587096514 @pingsutw, thanks for working on this! I took a look at the patch. Some comments/suggestions: First of all, I think this is a decent example to get data scientists started and allow us to add more models in the future. I like the simplicity brought by this example. **1) Regarding source code organization:** It is not very clear to me about what is entry point during review. I suggest following changes to the source code structure: - Put all deepFM.json/run_deepFM.py and data into submarine-sdk/pysubmarine/examples/deepfm - Move all deep_fm-related code into pysubmarine/submarine/ml to pysubmarine/submarine/model/deep_fm - There're already some common folders inside pysubmarine/submarine, like `utils`, etc. I think we should use them instead of creating new ones. - abstractModel can be moved to pysubmarine/submarine/model **2) Regarding code style** There're mixed style of namings in classes, functions and variables (like abstractModel.py, test_utils.py), we should make them consistent. **3) for model/deepFM.py** I'm wondering if it is from the original deep FM project, if yes I think we should add a comment to the file and point to the original Github link. 4) Add a README.md file into submarine-sdk/pysubmarine/example It is fine to make it super simple now, but it will be helpful to improve our examples library in the future. Thoughts?
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
