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

QAS Demo - Flower Model #273

Merged
merged 61 commits into from
Oct 18, 2023

Conversation

sebastian-echeverria
Copy link
Collaborator

This PR adds a second demo to the demo-2 folder with a flower model. Some notes and comments about it:

  • This demo is fully self-contained, so even if it has issues, it should not affect the rest of MLTE (low risk of merging PR, even if not fully reviewed)
  • The main goal of this demo is to show a use case where we start from a list of Quality Attribute Scenarios, and get to create a Spec, gather that data, and validate it in MLTE.
  • There is some how-to guidance being developed to guide through the demo (mostly on how to extend MLTE), but as it is the case with the original demo, it should be understandable in itself, and by default this guidance won't be inside the demo itself, so it shouldn't be a deterrent to move forward.
  • A secondary goal of this demo was to start with some real Jupyter Notebooks, and translate them to using MLTE, to find challenges in real use of MLTE. Some of these challenges generated changes already incorporated into MLTE, some are pending issues with suggestions of improvement.
  • In practice, related to the above, this demo makes use of adding new Properties outside of MLTE to add 4 new Properties needed for the QAS that are being validate. Several of these Properties should be merged into MLTE itself at some point, but for now having them isolated is simpler.
  • This demo also creates an Array value, which was very useful for most other Values that needed to be created to add validation methods for the different Conditions. Issue Create Array Value type #264 was created to evaluate integrating it into MLTE.
  • This demo contains several small data files (csv, a json model, and one image) that are used through the demo. None of them are big, but if we are not sure if we want to include all of them in the repo itself for different reasons, I can move them somewhere else and download them as needed (for example, not sure about the image permissions, or permissions for some of the CSV flower data). In particular, the actual model weights used for the demo is a huge 100 MB file that was instead uploaded to our CMU's Google Drive, and downloaded through a script during the demo execution. We may want to figure out if that is the proper place for that or other files as well.
  • There is a substantial overlap between this and the original demo (besides the overall structure). It may be beneficial at some point to merge both of them into one, but we can discuss this later.

sei-secheverria and others added 30 commits July 12, 2023 13:44
# Conflicts:
#	src/mlte/property/property.py
…ria/mlte into feature-new-properties

# Conflicts:
#	demo-2/evidence.ipynb
#	demo-2/garden.py
# Conflicts:
#	src/mlte/evidence/evidence_metadata.py
@sebastian-echeverria sebastian-echeverria changed the title QAS Demo QAS Demo - Flower Model Oct 4, 2023
@sebastian-echeverria
Copy link
Collaborator Author

@sebastian-echeverria sebastian-echeverria added this to the MLTE 0.2 Release milestone Oct 4, 2023
Copy link
Contributor

@krmaffey krmaffey left a comment

Choose a reason for hiding this comment

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

Sebastian, this demo is wonderful! It looks great to me, I think we are ready to merge it in. I discussed this with Kyle and he agrees, but we do have a request with regards to file structure.

Could you please change it so that this new demo is under the same folder as the old one? So the file structure will look something like this:

demo/
     simple/
          requirements.txt
          requirements.ipynb
          etc
     scenario/
          requirements.txt
          requirements.ipynb
          etc

I am not committed to those folder names (simple and scenario), if you have a better idea feel free to do that. My original thought was iris/ and flowers/ but I think it might be confusing since those are quite similar and the demos are distinct.

@turingcompl33t
Copy link
Collaborator

Agree with @krmaffey, just want to change the names of the directories now that we have two demonstrations, and this will be good to go!

@sebastian-echeverria
Copy link
Collaborator Author

Thanks! Ok, restructured folders to match that. Used your same folder names, just changed the second one to "scenarios" instead of "scenario". I made a few, minor modifications: added a note recommending to install requirements.txt for the simple demo in the negotiation card notebook, and rearrenged the gitignore files a bit to be more self-contained in the demo and reduce duplication between both folders.

Do note that if we move the model weights file for the "scenarios" demo, a minor change will be needed to the script that downloads it to download it from its new location.

Copy link
Contributor

@krmaffey krmaffey left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks Sebastian!

@krmaffey krmaffey merged commit ad22fe1 into mlte-team:master Oct 18, 2023
3 checks passed
@sebastian-echeverria sebastian-echeverria deleted the feature-new-properties branch October 18, 2023 17:10
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