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

issue #51: implement pipeline switching in back end #55

Merged
merged 52 commits into from
Apr 9, 2024

Conversation

MaxenceGui
Copy link

@MaxenceGui MaxenceGui commented Feb 8, 2024

@MaxenceGui MaxenceGui self-assigned this Feb 8, 2024
@MaxenceGui MaxenceGui added this to the M1 (2024 February) milestone Feb 8, 2024
@MaxenceGui MaxenceGui linked an issue Feb 8, 2024 that may be closed by this pull request
4 tasks
@MaxenceGui MaxenceGui marked this pull request as ready for review February 12, 2024 21:50
@MaxenceGui MaxenceGui requested review from rngadam and RussellJimmies and removed request for rngadam February 12, 2024 21:50
Copy link

@rngadam rngadam left a comment

Choose a reason for hiding this comment

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

app.py Outdated Show resolved Hide resolved
app.py Outdated Show resolved Hide resolved
Copy link

@rngadam rngadam left a comment

Choose a reason for hiding this comment

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

As discussed yesterday, the code here does not seem to be going in the right direction in terms of readability, testability and simplicity

app.py Outdated Show resolved Hide resolved
app.py Outdated Show resolved Hide resolved
@MaxenceGui
Copy link
Author

MaxenceGui commented Feb 22, 2024

I've added model documentation from the backend. It is still a work in progress. Following this doc, here is a list of what's left to do in the code:

  • Implement model categories with their task (Classification, Object detection, Segmentation)
  • Implement test on inference requests for different categories
  • Set up a JSON with the pipeline in the blob storage while the database is not yet ready

I'm asking for reviews from:

  • William (for your English skill and to keep you in the loop on Nachet)
  • Leron (to keep you in the loop)
  • Amir (to verify that I implemented your suggestion the good way)
  • François (to look at the metadata structure of model and pipeline)
  • Ricky

To access the new documentation: docs/nachet-model-documentation.md

Copy link

@rngadam rngadam left a comment

Choose a reason for hiding this comment

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

you have a number of lint failures reported from the Github Workflows.

Suggest managing your TODO list as a checklist in the description of this pull request (it's easy to lose track in the comments of the PR

Copy link

@rngadam rngadam left a comment

Choose a reason for hiding this comment

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

there's still basic issues with EOF newline missing so looks like the Github workflows are not all running.

app.py Outdated Show resolved Hide resolved
azure_storage/azure_storage_api.py Outdated Show resolved Hide resolved
azure_storage/azure_storage_api.py Outdated Show resolved Hide resolved
azure_storage/azure_storage_api.py Outdated Show resolved Hide resolved
azure_storage/azure_storage_api.py Outdated Show resolved Hide resolved
azure_storage/azure_storage_api.py Outdated Show resolved Hide resolved
azure_storage/azure_storage_api.py Outdated Show resolved Hide resolved
model_inference/model_module.py Outdated Show resolved Hide resolved
model_inference/model_module.py Outdated Show resolved Hide resolved
TESTING.md Outdated Show resolved Hide resolved
TESTING.md Outdated Show resolved Hide resolved
TESTING.md Outdated Show resolved Hide resolved
TESTING.md Outdated Show resolved Hide resolved
TESTING.md Outdated Show resolved Hide resolved
TESTING.md Outdated Show resolved Hide resolved
TESTING.md Outdated Show resolved Hide resolved
tests/test_inference_request.py Outdated Show resolved Hide resolved
tests/test_health_request.py Outdated Show resolved Hide resolved
model_inference/model_module.py Outdated Show resolved Hide resolved
@rngadam
Copy link

rngadam commented Mar 6, 2024

@MaxenceGui
New issues were created related to the work here so we can postpone some of the work and get this huge PR merged in main.

Modify the description to add checklist items as completed by referencing postponed tasks to new issues; this gives us a trace between the work that should have been done here and follow up issues.

@MaxenceGui
Copy link
Author

@MaxenceGui New issues were created related to the work here so we can postpone some of the work and get this huge PR merged in main.

Modify the description to add checklist items as completed by referencing postponed tasks to new issues; this gives us a trace between the work that should have been done here and follow up issues.

Perfect, will do that. I push my last commit so all checks pass. @RussellJimmies and @rngadam if there is any last-minute review you wanna add, go ahead. If you approve this PR I will rebase and push it to main.

run_tests.py Show resolved Hide resolved
@rngadam
Copy link

rngadam commented Mar 6, 2024

Seems like the issue links are created manually in the description? I think they look nicer if you use this notation:

* issue #59 
* issue #61 
* issue #60

output:

@MaxenceGui
Copy link
Author

Seems like the issue links are created manually in the description? I think they look nicer if you use this notation:

It is I will absolutely change it

azure_storage/azure_storage_api.py Outdated Show resolved Hide resolved
azure_storage/azure_storage_api.py Outdated Show resolved Hide resolved
Maxence Guindon and others added 13 commits April 8, 2024 14:18
fixes #51: Move insert_new_version_pipeline to pipelines_version_insertion.py
fixes #51: Refactor model folder
fixes #51: fix OEF

Issue #51: Correct typo
issue #51: change mermaid theme
Update CODEOWNERS file

Update CODEOWNERS file
* # fixes #64: Add MAX_CONTENT_LENGTH env

* fixes #64 Add devSecOps variable name

* fixes #64: fix typo

* Update README.md

Co-authored-by: Jonathan Lopez <[email protected]>

* Update .env.template

Co-authored-by: Jonathan Lopez <[email protected]>

* isssue #64: Get rid of eval

---------

Co-authored-by: Jonathan Lopez <[email protected]>

fixes #51: connecting to swin model

fixes #51: refactor exception

fixes #51: add request_factory function to bring a standard to call model

fixes #51: add models_Utils.py

fixes #51: Update model_utilitary_functions import

fixes #51: update result parsing and add default pipeline

fixes #51: update inference_request to call swin in loop

fixes #51: Standardize headers with azureml-model-deployment being mandantory

fixes #51: correct failed check from workflows

fixes #51: Refactor inference result processing and add test file for inference request

fixes #51: Change the categories model name

fixes #51: implement model module request inference function

fixes #51: add function to retrieve pipeline info from blob storage

fixes #51: Add script to upload to blob storage json file containing pipeline

fixes #51: correct lint error

Update nachet-inference-documentation.md to reflect code change

fixes #51: Update sequence diagram

fixes #51: update README and TESTING

fixes #51: correcting markdown lint

 fixes #51: Add inference request test

fixes #51: inference test with Quart.test_client

fixes #51: Correct lint ruff error and tests

fixes #51: implement Ricky reviews

Update CODEOWNERS file

Update CODEOWNERS file

fixes #64: Increase MAX_CONTENT_LENGTH (#66)

* # fixes #64: Add MAX_CONTENT_LENGTH env

* fixes #64 Add devSecOps variable name

* fixes #64: fix typo

* Update README.md

Co-authored-by: Jonathan Lopez <[email protected]>

* Update .env.template

Co-authored-by: Jonathan Lopez <[email protected]>

* isssue #64: Get rid of eval

---------

Co-authored-by: Jonathan Lopez <[email protected]>
issue #51: Give MAX_CONTENT_LENGTH default value

issue #51: Remove legacy folder
issue #51: Modify documentation to reflect change on Model namedtuple
* Update Sequence Diagram README.md

![SD_1 drawio (2)](https://github.com/ai-cfia/nachet-backend/assets/19809069/272f37dc-f4ec-449b-ba82-950c54b9f856)

issue #51: Fixing Markdown lint

issue #51: fixing MD lint
@MaxenceGui MaxenceGui force-pushed the 51-implementing-2-models branch from 2aee94c to d9ffdfb Compare April 8, 2024 14:23
@MaxenceGui
Copy link
Author

@RussellJimmies just missing your approval before merging in with main

Copy link
Contributor

@RussellJimmies RussellJimmies left a comment

Choose a reason for hiding this comment

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

Gargantuan work, great job! Code LGTM.

docs/nachet-inference-documentation.md Outdated Show resolved Hide resolved
docs/nachet-inference-documentation.md Outdated Show resolved Hide resolved
docs/nachet-inference-documentation.md Outdated Show resolved Hide resolved
docs/nachet-model-documentation.md Outdated Show resolved Hide resolved
Maxence Guindon and others added 4 commits April 9, 2024 15:51
@MaxenceGui MaxenceGui merged commit e8e4f8e into main Apr 9, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Implement Model Switching Functionality in Inference Request
6 participants