cog icon indicating copy to clipboard operation
cog copied to clipboard

added cog.yaml warning

Open asingh9530 opened this issue 1 year ago • 7 comments

What issue this PR fix - #1452

asingh9530 avatar Jan 05 '24 16:01 asingh9530

Thanks for the PR. It's a little confusing that this warning describes two possible issues. What do you think about handling the two exceptions with different warnings?

Some untested pseudocode:

except ConfigDoesNotExist:
        warnings.warn("no cog.yaml found")
except PredictorNotSet:
        warnings.warn("no predict() method found in Predictor")

zeke avatar Jan 05 '24 17:01 zeke

@zeke yeah I think that is much better let me make those changes.

asingh9530 avatar Jan 05 '24 17:01 asingh9530

So... this will print a warning, but it will also blow up because the pass was removed, right?

Is that that the intended behavior here, or do we want to print a warning without failing?

zeke avatar Jan 05 '24 18:01 zeke

So... this will print a warning, but it will also blow up because the pass was removed, right?

I think even in previous setting it would blow but without any warning or error but in this case it will show warning to user and then blow. So the user will likely know the root cause.

Is that that the intended behavior here, or do we want to print a warning without failing?

if we want to prevent the build to even begin then we have to raise exception instead of warning something like this.

    except ConfigDoesNotExist:
        raise ConfigDoesNotExist("no cog.yaml found or present")
    except PredictorNotSet:
        raise PredictorNotSet("no predict method found in Predictor")

what do you think ?

asingh9530 avatar Jan 05 '24 18:01 asingh9530

@zeke @aron can you please approve test workflows.

asingh9530 avatar Jan 09 '24 16:01 asingh9530

@zeke could please check why Integration failed as this PR dosen't contain any changes to the file reflected in tests.

asingh9530 avatar Jan 11 '24 13:01 asingh9530

@asingh9530 the test 'https://github.com/replicate/cog/blob/main/test-integration/test_integration/test_build.py#L9' fails because it tests for a specific error message, which has been changed in this PR. Please update the test to match the new error message.

yorickvP avatar Feb 08 '24 14:02 yorickvP

Thanks for your help with this, @asingh9530!

mattt avatar Jun 26 '24 12:06 mattt

Thanks for picking this up @mattt! 🙏🏼

zeke avatar Jul 02 '24 16:07 zeke