Skip to content

Conversation

@mthrok
Copy link
Contributor

@mthrok mthrok commented Jul 7, 2020

I anticipate some changes/improvements to sox_io backend which involves BC-breaking, so adding warning.

@mthrok mthrok requested a review from vincentqb July 7, 2020 15:15
Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

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

What changes are you expecting? If we are still expecting changes, we need to be careful how we expose new features and the bc-breaking changes to the users. Let's make sure all that is bc-breaking raises warnings that guide the users toward stable features, e.g. #761. (EDIT following comment)

In particular, should we then expose the backend as "_sox_io" ?

elif backend == 'sox':
module = sox_backend
elif backend == 'sox_io':
warnings.warn('"sox_io" backend is currently beta. Function signatures might change.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's say "experimental" instead of "beta". Should go as far as marking the backend as "_sox_io"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the difference between "beta" and "experimental"?
To me "beta" sounds it's ready to use but not in the final form yet, "experimental" sounds like we are playing around with it, which does not quite fit to my feeling. I could be wrong though.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have not defined either one of them formally, but torchtext used "experimental" :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, considering the feature lifecycle beta sounds more appropriate. The set of core features we want to provide is completed and confirmed to work. We are not going to change that, even though we refine some details and make small changes.

torchtext used "experimental"

That does not sound like a solid reasoning we should be based off of. That module is totally different nature.

@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #769 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #769   +/-   ##
=======================================
  Coverage   89.16%   89.17%           
=======================================
  Files          32       32           
  Lines        2566     2567    +1     
=======================================
+ Hits         2288     2289    +1     
  Misses        278      278           
Impacted Files Coverage Δ
torchaudio/backend/utils.py 89.36% <100.00%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad7f43f...4644825. Read the comment docs.

@mthrok
Copy link
Contributor Author

mthrok commented Jul 7, 2020

What changes are you expecting?

I am merging sox effects implementation and sox I/O implementation because they are practically same and can accomplish the same amount of things with less code. Along the way we might be able to get rid of some function arguments.

If we are still expecting changes, we need to be careful how we expose new features and the bc-breaking changes to the users.

That's the purpose of this PR.

Let's make sure all that is bc-breaking raises warnings that guide the users toward stable features, e.g. #761.

That will slow us down without much benefit, sox_io backend is still under development and we are still improving and refining it but these improvements come incrementally, therefore we need to break things. By stating publicly it's beta, we can break things more comfortably, yet allowing users to opt-in.
Raising warnings and such are for stable released feature and sox_io backend is not at the product lifecycle yet. The priority is to complete the feature and improve the quality before releasing it.

In particular, should we then expose the backend as "_sox_io" ?

What convention is that? That does not make BC breaking changes into non-BC breaking changes.

@vincentqb
Copy link
Contributor

What changes are you expecting?

I am merging sox effects implementation and sox I/O implementation because they are practically same and can accomplish the same amount of things with less code. Along the way we might be able to get rid of some function arguments.

That's cool :)

Let's make sure all that is bc-breaking raises warnings that guide the users toward stable features, e.g. #761.

That will slow us down without much benefit, sox_io backend is still under development and we are still improving and refining it but these improvements come incrementally, therefore we need to break things. By stating publicly it's beta, we can break things more comfortably, yet allowing users to opt-in.
Raising warnings and such are for stable released feature and sox_io backend is not at the product lifecycle yet. The priority is to complete the feature and improve the quality before releasing it.

Thanks for clarifying in comment. Since this is for sox_io, we indeed don't need to raise BC-breaking warning just yet. That being said, I agree with you we should find a way of communicating to the user that this is work in progress.

  • We do not advertise the sox_io backend until mature.
  • We expose only what we are reasonably confident won't change?
  • We use a alpha/beta/experimental warning or label.
  • Any other way?

In particular, should we then expose the backend as "_sox_io" ?

What convention is that? That does not make BC breaking changes into non-BC breaking changes.

I was really just brainstorming, and trying to see if there is another way of letting the user know that changes are coming. A better name than _sox_io to convey the same idea would be sox_io_experimental.

Thoughts?

@mthrok
Copy link
Contributor Author

mthrok commented Jul 8, 2020

I was really just brainstorming, and trying to see if there is another way of letting the user know that changes are coming. A better name than _sox_io to convey the same idea would be sox_io_experimental.

Thoughts?

I do not see a benefit of changing the name. It surely adds maintenance cost but it does not turn BC-breaking changes into non-BC-breaking. When such backend is removed users will have to change their code too. I believe Public beta feature is often what users can opt-in and if they are lucky enough not to be hit by BC-braking changes then they can keep using.

@mthrok
Copy link
Contributor Author

mthrok commented Jul 13, 2020

Superseded by #780 .

@mthrok mthrok closed this Jul 13, 2020
@vincentqb
Copy link
Contributor

As we discussed offline, we'll not expose the parameter that may change in the future, so as to remove the need for this warning.

@mthrok mthrok deleted the beta-sox-io branch July 14, 2020 16:49
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.

2 participants