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

version.model should return None if no model #276

Merged
merged 9 commits into from
Jul 2, 2024
Merged

Conversation

stellasphere
Copy link
Contributor

@stellasphere stellasphere commented Jun 27, 2024

Description

There should be no instance where version.model should return a model type class when there is no valid model present.

Fixing this issue: https://roboflow.slack.com/archives/C056ZUJPK08/p1719359598748219

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How has this change been tested, please provide a testcase or example of how you tested the change?

Locally
and here: https://colab.research.google.com/drive/1rnYv49x4oVWt--O3alw2wg6pr2qNapEO?usp=sharing

Any specific deployment considerations

For example, documentation changes, usability, usage/costs, secrets, etc.

Docs

  • Docs updated? What were the changes:

Copy link
Contributor

@LinasKo LinasKo left a comment

Choose a reason for hiding this comment

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

What I cannot evaluate is whether prior behavior, creating the model object without a matching model on the api, is valid use case somewhere.

roboflow/core/version.py Outdated Show resolved Hide resolved
roboflow/core/version.py Outdated Show resolved Hide resolved
roboflow/core/version.py Outdated Show resolved Hide resolved
@stellasphere
Copy link
Contributor Author

Thanks @LinasKo !

What I cannot evaluate is whether prior behavior, creating the model object without a matching model on the api, is valid use case somewhere.

I strongly believe that creating a model object when there is no model present is invalid behavior because the object it creates, a child of an InferenceModel, and its methods predict and load_model will fail because it doesn't exist. Doing so also makes it impossible to determine from the package that a model does or does not exist.

@LinasKo
Copy link
Contributor

LinasKo commented Jun 28, 2024

One other request: there's a test.py, which I think was included by accident.

1 similar comment
@LinasKo
Copy link
Contributor

LinasKo commented Jun 28, 2024

One other request: there's a test.py, which I think was included by accident.

Going to revoke key too
@stellasphere
Copy link
Contributor Author

Yup, I need to revoke my key too. Thanks for catching!

Copy link
Contributor

@LinasKo LinasKo left a comment

Choose a reason for hiding this comment

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

Approving, with the caveat that I did not run it locally, nor ran any tests.

@stellasphere
Copy link
Contributor Author

Sorry, fixed a failing test. @LinasKo can you re-approve?

@LinasKo
Copy link
Contributor

LinasKo commented Jun 28, 2024

M'afraid test.py snuck its way into the commit again 😉

What I'm seeing is - the model may not exist, but that's valid only in a project version where it's not been trained yet. Is that right?

@stellasphere
Copy link
Contributor Author

stellasphere commented Jun 28, 2024

M'afraid test.py snuck its way into the commit again 😉

🤦 You would think i would learn, but no.

the model may not exist, but that's valid only in a project version where it's not been trained yet. Is that right?

Yes, where it's not been trained. (or uploaded, but the same thing)

@stellasphere stellasphere requested a review from iurisilvio June 29, 2024 23:29
Copy link
Contributor

@iurisilvio iurisilvio left a comment

Choose a reason for hiding this comment

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

I agree there is the risk someone use this behaviour, but I think it should be fixed, "no model" is a known behavior and we have to return None to make it explicit.

LGTM! 🎉

@stellasphere stellasphere merged commit 20bdab5 into main Jul 2, 2024
8 checks passed
@stellasphere stellasphere deleted the fix-version-model branch July 2, 2024 01:06
@iurisilvio iurisilvio mentioned this pull request Jul 2, 2024
1 task
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