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

Improve deepaas-cli for OSCAR integration #158

Merged
merged 11 commits into from
Jul 19, 2024
Merged

Improve deepaas-cli for OSCAR integration #158

merged 11 commits into from
Jul 19, 2024

Conversation

IgnacioHeredia
Copy link
Collaborator

No description provided.

@IgnacioHeredia IgnacioHeredia added the enhancement New feature or request label May 6, 2024
@IgnacioHeredia IgnacioHeredia self-assigned this May 6, 2024
@IgnacioHeredia
Copy link
Collaborator Author

Hi @vykozlov ,

I have modified the deepaas-cli to prepare it to support OSCAR calls in the services deployed with PAPI.
Main changes:

  • fix error when field had no description
  • support for multiple file fields (reading fields.Field instead of using the field files name)
  • fix support for lists, bools and dict
  • improve help message

All these changes have been thoroughly tested with the demo_app which is using a wide variety of inputs.

This PR won't be merged right away (few tests still need to be done from OSCAR side), but it's mainly in a final shape.

Do the changes look good to you?

@vykozlov
Copy link
Contributor

Hi @IgnacioHeredia
thanks for updating, I will have a look, though need some time to refresh my memory :-)
One quick comment: if I recon correctly, "fields.Field" is a general class and we can't assume by default that every "fields.Field" is a file (function _get_file_args())

@IgnacioHeredia
Copy link
Collaborator Author

Hi @vykozlov ,

I agree, but there does not seem to be a field specific for files.

>>> dir(marshmallow.fields)
['And', 'AwareDateTime', 'Bool', 'Boolean', 'Constant', 'Date', 'DateTime', 'Decimal', 'Dict', 'Email', 'Enum', 'EnumType', 'Field', 'FieldABC', 'FieldInstanceResolutionError', 'Float', 'Function', 'IP', 'IPInterface', 'IPv4', 'IPv4Interface', 'IPv6', 'IPv6Interface', 'Inferred', 'Int', 'Integer', 'Length', 'List', 'Mapping', 'Method', 'NaiveDateTime', 'Nested', 'Number', 'Pluck', 'Raw', 'RemovedInMarshmallow4Warning', 'SchemaABC', 'Str', 'String', 'StringNotCollectionError', 'Time', 'TimeDelta', 'Tuple', 'URL', 'UUID', 'Url', 'ValidationError', '_Mapping', '_T', '__all__', '__builtins__', '__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__spec__', 'annotations', 'class_registry', 'collections', 'copy', 'decimal', 'dt', 'ipaddress', 'is_aware', 'is_collection', 'math', 'missing_', 'numbers', 'resolve_field_instance', 'types', 'typing', 'utils', 'uuid', 'validate', 'warnings']

Given that every other data type has it's own field type, I think it's fair to asume that we reserve fields.Field() for files. In fact that's already the default behaviour for deepaas: fields.Field arg is translated to a browse-file button in the Swagger UI.

Copy link

sonarcloud bot commented Jul 18, 2024

@alvarolopez alvarolopez merged commit 47d357f into master Jul 19, 2024
6 checks passed
@alvarolopez alvarolopez deleted the fix/cli branch July 24, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants