Skip to content

Simplify and clean up model loading system in deepaas/model/__init__.py - #201

Closed
alvarolopez with Copilot wants to merge 44 commits into
v3-fastapifrom
copilot/fix-2d2d6f9c-62cd-41c3-8eb8-8e63dfbd63cf
Closed

Simplify and clean up model loading system in deepaas/model/__init__.py#201
alvarolopez with Copilot wants to merge 44 commits into
v3-fastapifrom
copilot/fix-2d2d6f9c-62cd-41c3-8eb8-8e63dfbd63cf

Conversation

Copilot AI commented Sep 26, 2025

Copy link
Copy Markdown

This PR simplifies and cleans up the model loading system in deepaas/model/__init__.py to better match the single model use case and removes unnecessary complexity.

Changes Made

Legacy Code Removed

  • Removed the "extremely ugly" global variables pattern that was marked with FIXME(aloga): this is extremely ugly
  • Replaced the legacy load_v2_model() function with a cleaner register_v2_models(app) function

Simplified Implementation

  • Single Model Focus: Simplified the v2 module to use direct MODEL and MODEL_NAME variables instead of dictionary-based approach, since DEEPaaS only handles one model at a time
  • Cleaner Registration: Streamlined register_models() function to handle single model without unnecessary complexity
  • Direct Access: Implemented straightforward assignment in the main module for V2_MODEL and V2_MODEL_NAME
  • Maintained Compatibility: Preserved V2_MODELS dictionary interface through __getattr__ for API code that expects it

Backwards Compatibility

The changes maintain full backwards compatibility:

  • Existing API code that accesses model.V2_MODEL and model.V2_MODEL_NAME continues to work unchanged
  • API code expecting model.V2_MODELS dictionary interface still works
  • All existing functionality is preserved while significantly simplifying the underlying code

Before/After Comparison

Before (overcomplicated dictionary approach):

# Model registry
MODELS = {}
MODELS_LOADED = False

def register_models(app):
    global MODELS, MODELS_LOADED
    if MODELS_LOADED:
        return
    # ... complex dictionary management
    MODELS[model_name] = wrapper.ModelWrapper(...)
    MODELS_LOADED = True

After (simplified single model):

# Model registry - simplified for single model use case
MODEL = None  
MODEL_NAME = ""

def register_models(app):
    global MODEL, MODEL_NAME
    if MODEL:
        return
    # ... simple single model management
    MODEL = wrapper.ModelWrapper(...)
    MODEL_NAME = model_name

Testing

  • All existing tests pass (18/18)
  • Manual functionality tests confirm model loading and API creation work correctly
  • Both single model and dictionary access patterns verified working

This simplification removes unnecessary complexity while maintaining full functionality and better matching the actual single model use case.

Original prompt

Remove old legacy code in deepaas/model/__init__.py


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

This requires that we change all model args/responses from Marshmallow
to Pydantic. Most of the code is in this change, we can split it later
on two different changes (marshmallow + pydantic and FastAPI for
predict).
After checking with people using the package for a while, the training
methods were not used at all, therefore we are now removing the training
endpoints. This also allows us to have a much more simple codebase.
alvarolopez and others added 6 commits September 26, 2025 12:22
Co-authored-by: alvarolopez <468751+alvarolopez@users.noreply.github.com>
Co-authored-by: alvarolopez <468751+alvarolopez@users.noreply.github.com>
Co-authored-by: alvarolopez <468751+alvarolopez@users.noreply.github.com>
@alvarolopez

Copy link
Copy Markdown
Collaborator

@copilot the code that we used to manage the models and the model names, the registration, etc. can be improved, as we do not have to manage more than one single model now.

…_init__.py

Co-authored-by: alvarolopez <468751+alvarolopez@users.noreply.github.com>
Copilot AI and others added 2 commits September 26, 2025 12:10
… with MODELS dictionary

Co-authored-by: alvarolopez <468751+alvarolopez@users.noreply.github.com>
… V2_MODELS access

Co-authored-by: alvarolopez <468751+alvarolopez@users.noreply.github.com>
@alvarolopez

Copy link
Copy Markdown
Collaborator

@copilot I think you are using an outdated branch. Please rebase.

Copilot AI changed the title [WIP] Remove old legacy code in deepaas/model/__init__.py Remove legacy code in deepaas/model/__init__.py and modernize model loading system Sep 26, 2025
Copilot AI requested a review from alvarolopez September 26, 2025 12:14

Copilot AI commented Sep 26, 2025

Copy link
Copy Markdown
Author

@copilot I think you are using an outdated branch. Please rebase.

Rebased successfully from master branch and simplified the model management approach. The code now uses the current master branch as the base and implements a cleaner single model pattern as requested. All tests passing. Changes in commit 54d3439.

@copilot the code that we used to manage the models and the model names, the registration, etc. can be improved, as we do not have to manage more than one single model now.

I've simplified the model management code to handle only a single model as requested. Removed the dictionary-based MODELS = {} approach and reverted to a cleaner single MODEL and MODEL_NAME pattern. The code is now much simpler and more appropriate for the single model use case while maintaining all API compatibility. Changes implemented in commit 54d3439.

@sonarqubecloud

Copy link
Copy Markdown

Copilot AI changed the title Remove legacy code in deepaas/model/__init__.py and modernize model loading system Simplify and clean up model loading system in deepaas/model/__init__.py Sep 26, 2025
@alvarolopez

Copy link
Copy Markdown
Collaborator

@copilot I think you are using an outdated branch. Please rebase against v3-fastapi instead.

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.

3 participants