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

📖 Library Component Descriptions Update #180

Merged
merged 14 commits into from
Jul 29, 2022
Merged

Conversation

MFA-X-AI
Copy link
Member

@MFA-X-AI MFA-X-AI commented Jul 27, 2022

Description

With #171, we can now have rich component descriptions. This PR adds descriptions to all ~50 of them.

In general, this is the way it's formatted:

A short description of the component.

##### Reference: 
- A hyperlink to the reference

##### inPorts:
- inPort Name: short description on what it expects.
    Default: insert default value when needed.

##### outPorts:
- outPort Name: short description on what it outputs
    Default: insert default value when needed.

References

#171

Pull Request Type

  • Xircuits Core (Jupyterlab Related changes)
  • Xircuits Canvas (Custom RD Related changes)
  • Xircuits Component Library
  • Testing Automation
  • Documentation
  • Others (Please Specify)

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Tests

This would be a proof read PR. You'll need to check on all component descriptions and see if they make sense / formatted correctly. In general the steps would be:

  1. Install Xircuits wheel with the markdown render feature. Try this one https://github.com/XpressAI/xircuits/suites/7537756309/artifacts/310797122
  2. Fetch this PR's components xircuits-components --branch fahreza/update-desc
  3. Run Xircuits
  4. Drag each component one by one to the canvas, and check the rendering.

Tested on?

  • Windows
  • Linux Ubuntu
  • Centos
  • Mac
  • Others (State here -> xxx )

Notes

It's a tad cumbersome task to do, good luck. 😄

@MFA-X-AI MFA-X-AI requested a review from yuenherny July 27, 2022 09:22
@yuenherny
Copy link
Contributor

yuenherny commented Jul 27, 2022

The line in the Notes section is telling me to get rested for now - coz it would be impossible to get it done in 30 mins 🤣 (it's 5:32PM in Malaysia now)

@yuenherny
Copy link
Contributor

@MFA-X-AI Error in fetching xircuits components:

image

Perhaps its xircuits-components --branch fahreza/update-desc instead?

@MFA-X-AI
Copy link
Member Author

MFA-X-AI commented Jul 27, 2022

Yeah no worries, you can do it tomorrow in work hours.

Perhaps its xircuits-components --branch fahreza/update-desc instead?

Yep, thanks for pointing that out. I shamelessly copy pasted from the previous PR, didn't update it yet. 😄

@yuenherny
Copy link
Contributor

Comments on Xircuits Components - General Category

  1. Get Hyper-parameter String Name:
    image

    • Perhaps we could change "Enter String Value (Without Quotes)" to something else more intuitive - I am totally confused with what this component aims to do
      image
  2. Get Hyper-parameter Int Name, Get Hyper-parameter Float Name
    image
    image

  3. Get Hyper-parameter Boolean Name:
    image

    • Perhaps we could add "True" or "False" label on both sides of the toggle.
  4. Literal String, Integer, Float, True, False, List, Tuple, Dict:

    • Perhaps we could add "Create" and "Variable" in front and back of the component name respectively so that it is more intuitive - that these components allow users to create variables of different datatypes
    • e.g. "Literal String" -> "Create Literal String Variable"

At this point, I am also wondering if we could make the component details pop up when I hover my clicker on the component, before I click and drag into canvas - rather than user need to drag into canvas and then click on the "i" icon.

@MFA-X-AI
Copy link
Member Author

Oh, I wasn't expected you to review the general components as well since they're the hardbaked ones. Here's the docs about them:
https://xircuits.io/docs/technical-concepts/xircuits-components/literal-and-hyperparameters-components
Basically they're parameters to be supplied to the Library Components.

Technically they shouldn't have the information panel... but I guess if we can supply the information, that wouldn't be too bad as well. Which would be easier @AdrySky ?

I am also wondering if we could make the component details pop up when I hover my clicker on the component, before I click and drag into canvas - rather than user need to drag into canvas and then click on the "i" icon.

This is a good suggestion but will take some time to implement. We definitely can look into improving the component tray widget in the next version, including the ability to pip install component libraries from tray. 😄

@yuenherny
Copy link
Contributor

In regards to general components, perhaps we can point them to the doc like this:

A component to supply your Xircuits library components with customizable parameters, see link for more details.

This is because most of the users will just install and use stuff without even looking at the docs until they hit a roadblock. Hence, the component description itself will point them to the right place just about when they are confused what those general components do. 😄

@yuenherny
Copy link
Contributor

@MFA-X-AI Canvas zooms in and out when user is scrolling the component description. Should I file this as an Issue?

Xircuits LoadKerasModel

@MFA-X-AI
Copy link
Member Author

@yuenherny it is a known inconvenience which we haven't prioritized working on yet. You may create an issue if you'd like.

@AdrySky
Copy link
Contributor

AdrySky commented Jul 28, 2022

Technically they shouldn't have the information panel... but I guess if we can supply the information, that wouldn't be too bad as well. Which would be easier @AdrySky ?

Adding the descriptions is fine I guess. Might not be as straight forward to implement it.

This is a good suggestion but will take some time to implement. We definitely can look into improving the component tray widget in the next version, including the ability to pip install component libraries from tray.

Both are doable. But might take a while. Just create the tasks and if possible an issue as well.

When we have another new user, then you realized there's still a lot of works to be done T.T

@yusufraji
Copy link
Contributor

I noticed that using markdown heading level 3 (###) for the reference, inPort, and outPort doesn't look great. As those headings are way bigger than the heading used for the component name and description. Choosing a heading level similar/lower to the component name and description looks better IMO.

image

@MFA-X-AI
Copy link
Member Author

@yusufraji thanks for pointing that out! Now that I look at it, I guess h5 would fit better. Let me fix that.

@MFA-X-AI MFA-X-AI changed the title 📖 Component Descriptions Update 📖 Library Component Descriptions Update Jul 28, 2022
@yuenherny
Copy link
Contributor

Comments on Xircuits Components - Learning Category
P/s: Here, I'm assuming the technical details in the docstring are accurate.

  1. LoadImage: "A component to load images from dataset. User need to input dataset_name (Literal String) and it returns the dataset.
  2. ResizeImageData: "A component to resize dataset size or image size (not sure which one this component will do - can't find description anywhere in docs, and script doesn't open when I right click > Open Script)."
  3. LoadKerasModel: In args section, perhaps could add this line "Click link in Reference for more details."
  4. KerasPredict: In target_shape, perhaps modify to "np.array shape if not using default Keras model instance shape."
  5. ResNet50, 101, 152: Description for InPort args missing. Should be the similar to LoadKerasModel right?
  6. VGG16, 19: Looks good. No comments.
  7. Xception: Looks good. No comments.
  8. MobileNet: Perhaps the statement "This component returns... weights pre-trained on ImageNet." shan't be in the Reference section but in Description?
    image
  9. ReadDataSet: Perhaps include an example on how to specify a directory path in description - is it absolute path or relative path?
  10. ReadMaskDataSet: No description provided. Perhaps not a priority to have one now?
  11. FlattenImageData: Looks good. No comments.
  12. TrainTestSplit: Suggest changing "Scikitlearn" to "Scikit-learn"; description for dataset in inPorts section isn't complete?
  13. Create1DInputModel: Formatting for inPorts and outPorts looks off. Might need to add h5 formatting in it.
  14. Create2DInputModel: Formatting for arguments inPorts and outPorts looks off. Might need to format it with bullet points.
  15. TrainImageClassifier: Looks good. No comments.
  16. EvaluateAccuracy: Looks good. No comments.
  17. ShouldStop: Incomplete description in inPorts's metric - "dict that contains results of ???" - I guess its "evaluation"
  18. SaveKerasModel: Looks good. No comments.
  19. SaveKerasModelInModelStash: No description. Perhaps not a priority for now.

That's all for Learning category. Moving on to Pytorch category now.

@yuenherny
Copy link
Contributor

yuenherny commented Jul 28, 2022

Comments on Xircuits Components - Pytorch Category
P/s: Here, I'm assuming the technical details in the docstring are accurate.

  1. LoadTorchVisionDataset: LGTM.
  2. TorchDataLoader: LGTM.
  3. TorchModel: Missing inPorts section. Maybe can put None under inPorts section.
  4. TrainTorchModel, TestTorchModel: The term torch nn is kinda weird to see, might be the OCD in me poking around - I prefer torch.nn 😆. I can help with these minor improvements if you don't mind.
  5. SaveTorchModelState, LoadTorchModelState: Perhaps we can give examples to save using relative and absolute path?
  6. TorchModelPredict, TorchModelPredictFromTensor: Suggest class_list description to be changed to "a list of output classes."
  7. Image2TorchTensor: LGTM.

That's all for Pytorch category. Skipping Spark category as I think its easier to finish off Template and Utils before 6pm.

@yuenherny
Copy link
Contributor

Comments on Xircuits Components - Template Category
P/s: Here, I'm assuming the technical details in the docstring are accurate.

  1. HelloComponent, HelloHyperparameter, CompulsoryHyperparameter, : LGTM.
  2. HelloListTupleDict: Perhaps more description on the usage of this component?
  3. HelloContext: Perhaps more description on the usage of this component and how users implement it in canvas?
  4. AddImport: Maybe the examples should showcase adding imports instead of adding print("NEW LINE ADDED") so that users know how to use it. Then specify that users can use this tool to add code other than imports too.

Comments on Xircuits Components - Utils Category
P/s: Here, I'm assuming the technical details in the docstring are accurate.

  1. ZipDirectory: Suggest to change the zip_fn argument to filename. Then description will be "the name of the zipped file. Default: .xircuits canvas name." The EG section is a nice touch.
  2. DeleteFile: LGTM.
  3. TimerComponent: Perhaps a Reference section linking to a tutorial on how to use this component in canvas.
  4. SleepComponent: LGTM.

That's all for Template and Utils category. Will continue on with Spark category after this.

@yuenherny
Copy link
Contributor

Comments on Xircuits Components - Spark Category
P/s: Here, I'm assuming the technical details in the docstring are accurate.

  1. xSparkSession: Missing semicolon in appname - "Default: .xircuits canvas name" would be more readable.
  2. SparkReadPandas: Perhaps include links in Reference section, if any
  3. SparkReadCSV, SparkReadFile: In file_input, suggest to include examples for absolute and relative path, Perhaps include links in Reference section, if any
  4. SparkSQL: Incoherent description in table_name - specify a table name "that" is already created by createOrReplaceTempView? Perhaps include links in Reference section
  5. SparkVisualize: LGTM
  6. SparkSparseVector: LGTM
  7. SparkLabeledPoint: Suggest to bold "Only the sparse vector input be used if both are supplied"
  8. SparkLoadImageFolder: In folder_path, suggest to include examples for absolute and relative path
  9. SparkSplitDataFrame: LGTM
  10. SparkLoadLIBSVM: Suggest description "Loads a libsvm file into a spark dataframe"; In folder_path, suggest to include examples for absolute and relative path; Suggest to include link to read.format() config in References and examples onl how to supply those params - is it via dict or list?
  11. SparkPredict: LGTM

That's all for Spark category. Lemme know if you need help with rectifying these comments. Overall great work with the docstrings!

Copy link
Contributor

@yuenherny yuenherny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completed review. Overall, good work with the docstrings.
Please do take a look at the comment thread in PR for details.

Once that done, should be good to merge.

@MFA-X-AI
Copy link
Member Author

MFA-X-AI commented Jul 29, 2022

Thanks for the through review, I've updated the components based on your feedback.

Some of the stuff I've noted:

  • The AddImport component is indeed one of a kind! Will add a section in the documentations for it since it's difficult to show in a few words.

  • suggest to include examples for absolute and relative path
    I've noticed that you've suggested this multiple times, I wasn't very sure what to include here. FYI when you launch jupyterlab, the place where you launch it becomes the root, and every path will be as a relative to it. Similarly, Jupyterlab users can only use the File Browser to access their own files and directories. This is a good security design so regular users have limited scope.

    Anyways, the easiest way to know where the relative path for a particular file in the filebrowser is to fetch the file path in the file browser.
    paths

  • SparkLoadLIBSVM Suggest to include link to read.format() config in References

This was actually much harder than I thought. 😄 I've gotten the reference on how to load the libsvm from this example but lo and behold, even the official docs only provide the description as "all other string options". It would likely refer to this, however as we don't have a proper use case for it currently, I'm removing that port altogether.

One of the thing that I'm on the fence about is if we would want to modify our ports to be like how the spark docs does it:

        Parameters
        ----------
        path : str, optional
            the path in a Hadoop supported file system
        format : str, optional
            the format used to save
        mode : str, optional
            specifies the behavior of the save operation when data already exists.

            * ``append``: Append contents of this :class:`DataFrame` to existing data.
            * ``overwrite``: Overwrite existing data.
            * ``ignore``: Silently ignore this operation if data already exists.
            * ``error`` or ``errorifexists`` (default case): Throw an exception if data already \
                exists.
        partitionBy : list, optional
            names of partitioning columns
        **options : dict
            all other string options

It nicely mentions whether ports are optional or not and also the expected in type. For that reason, I'm not deleting the branch yet, just in case we'd like to update it to this format.

@MFA-X-AI MFA-X-AI merged commit f1df0cd into master Jul 29, 2022
@MFA-X-AI MFA-X-AI deleted the fahreza/update-desc branch August 5, 2022 03:17
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.

4 participants