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

Make serverless function naming and Docker image naming consistent #6140

Merged
merged 1 commit into from
May 26, 2023

Conversation

SpecLad
Copy link
Contributor

@SpecLad SpecLad commented May 12, 2023

For function names, take the relative path, lowercase, join with dashes and replace underscores with dashes.

For image names, take the relative path, lowercase and join with dots.

In a couple cases, rename the function directory instead of fixing the config.

Motivation and context

All these inconsistencies are driving me batty. Derived names should be derived the same way everywhere in order to be predictable.

How has this been tested?

Checklist

  • I submit my changes into the develop branch
  • [ ] I have added a description of my changes into the CHANGELOG file
  • I have updated the documentation accordingly
  • [ ] I have added tests to cover my changes
  • [ ] I have linked related issues (see GitHub docs)
  • [ ] I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

License

  • ] I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@SpecLad
Copy link
Contributor Author

SpecLad commented May 12, 2023

The idea is originally from #6129, but I fixed all functions this time, not just OpenVINO ones.

Here's the script I used to detect mismatches:

import yaml

from pathlib import Path

for config_path in Path('serverless').glob("**/*.yaml"):
    config_rel = config_path.relative_to('serverless')

    assert config_rel.parent.name == 'nuclio'

    parts = list(config_rel.parents[1].parts)

    if parts[0] == 'pytorch':
        parts[0] = 'pth'
    elif parts[0] == 'tensorflow':
        parts[0] = 'tf'

    with config_path.open('rb') as config_file:
        config = yaml.safe_load(config_file)

    expected_name = '-'.join(part.replace('_', '-').lower() for part in parts)
    actual_name = config['metadata']['name']

    if actual_name != expected_name:
        print(f'{config_path}: name mismatch: expected {expected_name!r} actual {actual_name!r}')

    expected_image_name = 'cvat.' + '.'.join(part.lower() for part in parts)
    actual_image_name = config['spec']['build']['image']

    if actual_image_name != expected_image_name:
        print(f'{config_path}: image name mismatch: expected {expected_image_name!r} actual {actual_image_name!r}')

Probably should add something like this to CI, but no time for that right now.

@bsekachev
Copy link
Member

Quick look at our documentation shows that we still have some references using previous naming.
For example:

image

Also, If we rename SAM, we need to update our SAM article: https://www.cvat.ai/post/facebook-segment-anything-model-in-cvat (better to collaborate with Maria to know if we published it somewhere else).

And just thoughts, this patch will result our users have several deployed models with the same name in CVAT. Maybe it is not a big problem.

@SpecLad
Copy link
Contributor Author

SpecLad commented May 23, 2023

Quick look at our documentation shows that we still have some references using previous naming.

I believe those are all referring to the TensorFlow version (which hasn't changed).

Also, If we rename SAM, we need to update our SAM article

Yeah, that's a fair point. I don't want to deal with it right now, so I just reverted the SAM changes. I might return to it later.

And just thoughts, this patch will result our users have several deployed models with the same name in CVAT. Maybe it is not a big problem.

I don't think it's a big problem (especially given that I already updated a whole bunch of names in #6100), but just in case, I added a changelog entry describing the name changes.

For function names, take the relative path, lowercase, join with dashes and
replace underscores with dashes.

For image names, take the relative path, lowercase and join with dots.

In a couple cases, rename the function directory instead of fixing the
config.

SAM still violates the convention; keep it as-is for now, as to not break
the references from published articles.
@nmanovic nmanovic merged commit a5cbd1a into cvat-ai:develop May 26, 2023
@azhavoro azhavoro mentioned this pull request Jun 2, 2023
nmanovic added a commit that referenced this pull request Jun 2, 2023
## \[2.4.5] - 2023-06-02
### Added
- Integrated support for sharepoint and cloud storage files, along with
directories to be omitted during task creation (server)
(<#6074>)
- Enabled task creation with directories from cloud storage or
sharepoint (<#6074>)
- Enhanced task creation to support any data type supported by the
server
by default, from cloud storage without the necessity for the `use_cache`
option (<#6074>)
- Added capability for task creation with data from cloud storage
without the `use_cache` option
(<#6074>)

### Changed
- User can now access resource links from any organization or sandbox,
granted it's available to them
(<#5892>)
- Cloud storage manifest files have been made optional
(<#6074>)
- Updated Django to the 4.2.x version
(<#6122>)
- Renamed certain Nuclio functions to adhere to a common naming
convention. For instance,
`onnx-yolov7` -> `onnx-wongkinyiu-yolov7`, `ultralytics-yolov5` ->
`pth-ultralytics-yolov5`
  (<#6140>)

### Deprecated
- Deprecated the endpoint `/cloudstorages/{id}/content`
(<#6074>)

### Fixed
- Fixed the issue of skeletons dumping on created tasks/projects
(<#6157>)
- Resolved an issue related to saving annotations for skeleton tracks
(<#6075>)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Boris Sekachev <boris.sekachev@yandex.ru>
Co-authored-by: Roman Donchenko <roman@cvat.ai>
Co-authored-by: Maria Khrustaleva <maya17grd@gmail.com>
Co-authored-by: Boris Sekachev <sekachev.bs@gmail.com>
Co-authored-by: Nikita Manovich <nikita@cvat.ai>
Co-authored-by: Anastasia Yasakova <yasakova_anastasiya@mail.ru>
Co-authored-by: Snyk bot <snyk-bot@snyk.io>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Kirill Sizov <kirill.sizov@cvat.ai>
Co-authored-by: Paweł Kotiuk <kotiuk@zohomail.eu>
Co-authored-by: SK <450723+senthilkumarkj@users.noreply.github.com>
Co-authored-by: Kirill Lakhov <kirill.9992@gmail.com>
mikhail-treskin pushed a commit to retailnext/cvat that referenced this pull request Jul 1, 2023
…vat-ai#6140)

For function names, take the relative path, lowercase, join with dashes
and replace underscores with dashes.

For image names, take the relative path, lowercase and join with dots.

In a couple cases, rename the function directory instead of fixing the
config.
@SpecLad SpecLad deleted the function-consistency branch July 20, 2023 10:14
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