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

Enhancements to Repo #10

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Enhancements to Repo #10

wants to merge 1 commit into from

Conversation

JonoCX
Copy link
Member

@JonoCX JonoCX commented Nov 9, 2022

This is all good work. It's worth saying that this PR isn't designed to be merged, but rather an easier place for me to document suggestions - it may work out better to address the changes below, push to this PR, and then I re-review but we can talk about that separately.

The repository itself is quite messy and needs some refinement - I know you're still working on it. The structure needs to be thought about more carefully and you should put yourself in the position of a client, i.e., how easy is their journey through this guide/tutorial?

In addition, these patterns should showcase our ability to write clean, well-documented code, with easy to understand and follow documentation.

To build on the points above in more detail, here are the changes I think you should make:

  • There are utility fles/functions stored in the data directory which is quite messy - these should be either moved into the utils directory or integrated into the 'pipeline' (pick one). Raises a question as to whether there is a need for a Dataset object?
  • We should only need a single file to fun the entire pattern (e.g., run_demo.py), calling on the various functions that perform the particular steps.
  • As is done in other tutorial-esque repo's, the notebook(s) should have their own separate directory.
  • Rather than having model_server and monitoring_server, can we have a single server directory which contains two sub-directories called model and monitoring?
  • In a lot of the Python files, the majority of the code is run within the program execution if statement - this isn't the best practice and looks messy. You should aim to make your code more functional (and readable) by separating out specific bits of functionality into their own functions (it makes testing significantly easier). I've left a few comments in the code highlighting this.
  • You don't say that you need Docker running prior to running the demo, just that you need it.
  • It's not entirely clear what I need to do once I have the demo running - walk the customer through it.
  • The transparent background on the diagram in 'how does the demo work?' isn't visible when you're in 'dark' ode (you can only see the white boxes).
  • I think you can change the readme to be much simpler and focus much more on what this pattern is and what you need to run it. You have a docs folder, use that to include the more detailed descriptions about how it works, etc., and point to them in the main readme.
  • There is a mix match of comments, in some functions they're fully commented and in others, they're not.
  • Nit: Similarly, the language style of the readme could be more in the style of the blogs and make sure you proof read the readme's.

There’s a lot there and it’s mostly nitpicking. These patterns want to showcase our ability so they need to be great. Happy to chat about any points, and I’ll update with anything else I spot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant