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

Public API contract #55

Closed
jeremyfowers opened this issue Dec 4, 2023 · 3 comments · Fixed by #65
Closed

Public API contract #55

jeremyfowers opened this issue Dec 4, 2023 · 3 comments · Fixed by #65
Assignees
Labels
cleanup Cleaning up old/unused code and tech debt p0 Top priority
Milestone

Comments

@jeremyfowers
Copy link
Collaborator

jeremyfowers commented Dec 4, 2023

Introduction

This issue describes a public API contract that will be enshrined into code.

The public API is:

  • Any class, function, or data structure that is meant to be used by external code (e.g., wrapper utilities, plugins, etc.) is exposed via src/turnkeyml/__init__.py
  • Any breaking change to the interface of anything in src/turnkeyml/__init__.py constitutes a breaking change to TurnkeyML and a rev of the major version number
  • Changes that don't change an interface in src/turnkeyml/__init__.py is not considered a breaking change.
  • External code uses classes/functions/data NOT in the public API at its own risk. Those internal classes/functions/data can be changed without warning.

Contract

The following classes, functions, and data are already in the public API:

  • turnkeycli: the turnkey CLI
  • benchmark_files()
  • build_models()
  • load_state() API for loading build state
  • The package version number turnkeyml.version

We will be adding the following:

  • From the run module:
    • The BaseRT class
  • From the common.filesystem module:
    • get_available_builds()
    • make_cache_dir()
    • MODELS_DIR: the location of turnkey's corpus on disk
    • Stats
    • Keys: the common keys for the Stats dicts
  • From the common.printing module:
    • log_info()
    • log_warning()
    • log_error()
  • From the build.export module:
    • onnx_dir()
    • ExportPlaceholder(Stage)
    • OptimizeOnnxModel(Stage)
    • ConvertOnnxToFp16(Stage)
  • From the build.stage module:
    • Sequence
    • Stage
  • From the common.build module:
    • State
    • logged_subprocess() BTW this is only ever used in runtime plugins, so why is it in the build module?? Will move it...
  • From the common.exceptions module:
    • StageError
    • ModelRuntimeError
  • From run.plugin_helpers everything
    • CondaError
    • SubprocessError
    • get_python_path()
    • run_subprocess()
    • HardwareError

Not in contract

There are some helper functions used in external tests that I dont want to expose in the public API. They just dont seem like "official turnkey functions" because they are too generic. They are:

In the filesystem module:

  • get_all(): finds all the files with a given file extension
  • rmdir(): delete a directory from the filesystem
  • expand_inputs()

In the export module:

  • check_model(): just a wrapper for onnx.checker..check_model()
  • base_onnx_file(): location on disk of the fp32 onnx file (this shouldnt be public info; Stages should only get to know where the intermediate result is, not a specific past result)

My advice would be for external code to either copy the internal code or replace it with something more generic.

@jeremyfowers jeremyfowers added cleanup Cleaning up old/unused code and tech debt p0 Top priority labels Dec 4, 2023
@jeremyfowers jeremyfowers added this to the Version 1.0 milestone Dec 4, 2023
@jeremyfowers jeremyfowers self-assigned this Dec 4, 2023
@jeremyfowers
Copy link
Collaborator Author

@danielholanda @ramkrishna2910 one more thing to consider: in our semantic versioning, we define:

  • a major version change as anything that would make people change their code
  • a minor version change as anything that would require a rebuild

I could be wrong, but due to the advent of the plugin API, there are vanishingly few cases where the minor version number would be updated instead of the major version number. For example, if I change a field in State that will trigger rebuilds... but also be a breaking API change to plugin developers.

Should we change the semantic versioning?

  • Major: huge overhaul of the product (e.g., "We are finally announcing v1!" "We are adding 7 new plugins so that is v2!"
  • Minor: API breaking change
  • Patch: Doesn't qualify as either of the above

Thoughts?

@danielholanda
Copy link
Collaborator

Your versioning system seems to mix elements of semantic versioning with additional marketing-driven changes (major version changes being defined by big announcements).

The major version in semantic versioning usually indicates backward-incompatible changes. The proposed major version seems to be more aligned with major milestones or significant marketing events, which might not strictly adhere to the semantic versioning rules.

That being said, I'm okay to have a customized versioning approach that suits our needs, even if it doesn't strictly adhere to semantic versioning standards.

@jeremyfowers
Copy link
Collaborator Author

jeremyfowers commented Dec 5, 2023

Actions:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Cleaning up old/unused code and tech debt p0 Top priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants