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

Deprecate older VGG API #270

Merged
merged 6 commits into from
Jan 5, 2024
Merged

Deprecate older VGG API #270

merged 6 commits into from
Jan 5, 2024

Conversation

theabhirath
Copy link
Member

Fix #269. Also adds the pre-trained API to the docstring of VGG. And a small rewrite of the VGG block, which was written pre-conv_norm's current more powerful form.

Right now the depwarn isn't printed by default, should we force it to be true? Or is it reasonable to assume that people will spot this and not upgrade if they don't want to/migrate to the newer API if they want to?

@theabhirath theabhirath added the documentation Documentation label Jan 4, 2024
Copy link
Contributor

github-actions bot commented Jan 4, 2024

Once the documentation build has completed, you can preview any updated documentation at this URL: https://fluxml.ai/Metalhead.jl/previews/PR270/

@theabhirath
Copy link
Member Author

P.S. there has to be a way to limit the tests run when we open a PR, because running ResNet/MobileNet tests for a VGG PR seems hugely wasteful. Is there a hook to GH Actions that can let us specify which tests to run via a comment or something?

src/convnets/vgg.jl Outdated Show resolved Hide resolved
src/convnets/vgg.jl Outdated Show resolved Hide resolved
@darsnack
Copy link
Member

darsnack commented Jan 4, 2024

I disagree—passing tests always seem wasteful 😁. IMO tests that don't always run are as bad as no tests at all.

Co-Authored-By: Kyle Daruwalla <daruwal@cshl.edu>
@theabhirath
Copy link
Member Author

I disagree—passing tests always seem wasteful 😁. IMO tests that don't always run are as bad as no tests at all.

That makes sense 😂 I was thinking more from the GH actions perspective of how many free minutes we get but I think the individual test sets are executing pretty quickly now so it should be okay

@darsnack
Copy link
Member

darsnack commented Jan 4, 2024

Public repos have unlimited minutes, so that's not an issue. I guess the only thing is that a single org shares runners across all repos, and there are a fixed amount of runners you can use in parallel. But I haven't noticed this to be an issue; it's not like we are waiting significantly longer to merge PRs.

Shorter turnaround time is always nicer, but I say we target the actual source of latency for now.

@theabhirath
Copy link
Member Author

Public repos have unlimited minutes, so that's not an issue.

Oh, for some reason I thought this was limited 😅 That's perfect then, waiting a bit for a PR to merge is not a problem at all.

@theabhirath theabhirath requested a review from darsnack January 5, 2024 01:47
@theabhirath theabhirath merged commit eed0c64 into master Jan 5, 2024
42 checks passed
@theabhirath theabhirath deleted the a2/vgg-fix branch January 5, 2024 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VGG has two "higher-level" functions
2 participants