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

[16.0][IMP] storage_backend: New implementation #250

Closed
wants to merge 8 commits into from

Conversation

lmignon
Copy link
Sponsor Contributor

@lmignon lmignon commented Mar 24, 2023

Simplify dependency tree by removing the dependency on server_environment and component.

Add support for more backend types and provides a wider api. By relaying on the fsspec python library, we've now access to a wider range of supported filesystem like systems and a richer api

fixes #224

todo:

  • Fix ui for protocol options
  • Provides documentation on how to configure protocols and how some can be nested.
  • Bridge the transactional system (Added to the roadmap)
  • rename to filesystem

@lmignon lmignon added this to the 16.0 milestone Mar 24, 2023
@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Mar 24, 2023

ping @sebastienbeau

Simplify dependency tree by removing the dependency on server_environment and component.

Add support for more backend types and provides a wider api. By relaying on the fsspec python library, we've now access to a wider range of supported filesystem like systems and a richer api
@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Mar 25, 2023

@sebastienbeau @HviorForgeFlow @santostelmo @simahawk With this new approach, we no more need to specific storage_backend_(sftp|s3| ....) since the python package used to get access to a filesystem like interface supports natively a wide range of protocol (see fsspec)

IMO the name storage_backend does not describe well the function of this addon.
I propose to rename it filesystem. The others companion addons could be renamed to replace the prefix storage with fs.

What do you think about?

@rvalyi
Copy link
Member

rvalyi commented Mar 25, 2023

Hello @lmignon this looks terrific. I have a question though: did you think about a use case where we would make the default Odoo File System actually use fsspec itself? I see you at Acsone made a module to have Odoo sessions in the database, I guess you would have as much interest into putting the Odoo filestore in a cloud file system, right? Will it be compatible with the way you refactored storage_backend?

@lmignon lmignon marked this pull request as ready for review March 25, 2023 15:07
@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Mar 25, 2023

Hello @lmignon this looks terrific. I have a question though: did you think about a use case where we would make the default Odoo File System actually use fsspec itself? I see you at Acsone made a module to have Odoo sessions in the database, I guess you would have as much interest into putting the Odoo filestore in a cloud file system, right? Will it be compatible with the way you refactored storage_backend?

Hi @rvalyi

The original Idea comes from @sebastienbeau and indeed after digging with fsspec and its use as base for the storage addons, I'm very impressed by the possibilities such approach offers. It could make sense to use the same approach when we need to store the filestore in a cloud platform but you've to keep in mind that not all the files from the filestore could be stored outside of the odoo file system. (I refers here to the files generated for the assets etc...)
For the sessions storage, the story is different, we need to provide a very efficient way to store and retrieve the related files. IMO, this efficiency is only possible by providing the right algorithm according the chosen strategy. If you take a loock to the addon that allows you to store these sessions into a database, you'll see that this approach is very particular. Moreover, the session management occurs at server level not at database level.

@hparfr
Copy link
Contributor

hparfr commented Mar 27, 2023

Looks promising

, we no more need to specific storage_backend_(sftp|s3| ....) since the python package used to get access to a filesystem like interface supports natively a wide range of protocol (see fsspec)

Having dedicated module may be necessary to ensure the python lib is installed fspec[s3] and to have an UI to configure the backend.

On runbot, I have some duplicate :

image

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Mar 27, 2023

Having dedicated module may be necessary to ensure the python lib is installed fspec[s3] and to have an UI to configure the backend.

You don't need a specific UI to configure the filesystem. The configuration must be provided into the options field. Additional dependencies are to be set at project level into your pyproject.yaml file our into the requiremetns.txt of your project our installed manually

On runbot, I have some duplicate :

This will be fixed. Some protocol share the same implementation class.... and I display the implementation class name as label for protocols. I'll improve the displayed label.

@hparfr
Copy link
Contributor

hparfr commented Mar 29, 2023

The configuration must be provided into the options field. Additional dependencies are to be set at project level into your pyproject.yaml file our into the requiremetns.txt of your project our installed manually

Ok

You don't need a specific UI to configure the filesystem.

Yes, but we may add one optionnal UI module at some point to ease the configuration and allow broader adoption.

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Mar 29, 2023

You don't need a specific UI to configure the filesystem.

Yes, but we may add one optionnal UI module at some point to ease the configuration and allow broader adoption.

Yes could do it but IMO the audience for this kind of addon is advanced/technicals users. This kind of configuration is done one time and will almost never change. At least, the current implementation give place for additional addon that could provide a richer UI....

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Mar 29, 2023

@hparfr You could also take a look at #252 where I put a document to explain what I plan to do with others addon.

Copy link
Member

@HviorForgeFlow HviorForgeFlow left a comment

Choose a reason for hiding this comment

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

I started testing the new implementation running on python 3.8.15 this error prompts:

2023-04-13 13:57:15,523 148282 ERROR 16.0-refactor-storage-lmi odoo.modules.registry: Failed to load registry 
2023-04-13 13:57:15,523 148282 CRITICAL 16.0-refactor-storage-lmi odoo.service.server: Failed to initialize database `16.0-refactor-storage-lmi`. 
Traceback (most recent call last):
  File "/home/hvior/Projects/odoo/16.0/odoo/odoo/service/server.py", line 1299, in preload_registries
    registry = Registry.new(dbname, update_module=update_module)
  File "<decorator-gen-14>", line 2, in new
  File "/home/hvior/Projects/odoo/16.0/odoo/odoo/tools/func.py", line 87, in locked
    return func(inst, *args, **kwargs)
  File "/home/hvior/Projects/odoo/16.0/odoo/odoo/modules/registry.py", line 90, in new
    odoo.modules.load_modules(registry, force_demo, status, update_module)
  File "/home/hvior/Projects/odoo/16.0/odoo/odoo/modules/loading.py", line 483, in load_modules
    processed_modules += load_marked_modules(cr, graph,
  File "/home/hvior/Projects/odoo/16.0/odoo/odoo/modules/loading.py", line 371, in load_marked_modules
    loaded, processed = load_module_graph(
  File "/home/hvior/Projects/odoo/16.0/odoo/odoo/modules/loading.py", line 188, in load_module_graph
    load_openerp_module(package.name)
  File "/home/hvior/Projects/odoo/16.0/odoo/odoo/modules/module.py", line 432, in load_openerp_module
    __import__('odoo.addons.' + module_name)
  File "/home/hvior/Projects/odoo/16.0/addons/OCA/storage/storage_backend/__init__.py", line 6, in <module>
    from . import models
  File "/home/hvior/Projects/odoo/16.0/addons/OCA/storage/storage_backend/models/__init__.py", line 1, in <module>
    from . import storage_backend
  File "/home/hvior/Projects/odoo/16.0/addons/OCA/storage/storage_backend/models/storage_backend.py", line 66, in <module>
    class StorageBackend(models.Model):
  File "/home/hvior/Projects/odoo/16.0/addons/OCA/storage/storage_backend/models/storage_backend.py", line 143, in StorageBackend
    def _get_protocols(self) -> List[tuple[str, str]]:
TypeError: 'type' object is not subscriptable

Adding the Tuple on the typing import and changing the in built tuple by the imported Tuple allow me to install the module.

import warnings
from typing import AnyStr, List
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from typing import AnyStr, List
from typing import AnyStr, List, Tuple

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

@HviorForgeFlow Thank you for the tests. Can you try by adding

from __future__ import annotations

I am actively developing the new approach explained here #252 In this draft PR you'll see that I renamed the storage_backend addon into fs_storage and improved it . I also created a new PR #260 where I add the new fs_attachment addon. This addon will be the base of the next new addon fs_file that will replace storage.file .

return super().write(vals)

@api.model
def _get_protocols(self) -> List[tuple[str, str]]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def _get_protocols(self) -> List[tuple[str, str]]:
def _get_protocols(self) -> List[Tuple[str, str]]:

rec.protocol_descr = fsspec.get_filesystem_class(rec.protocol).__doc__

@api.model
def _get_options_protocol(self) -> List[tuple[str, str]]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def _get_options_protocol(self) -> List[tuple[str, str]]:
def _get_options_protocol(self) -> List[Tuple[str, str]]:

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Jun 28, 2023

superseded by #252

@lmignon lmignon closed this Jun 28, 2023
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.

None yet

4 participants