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

Remove warning about compound tags deprecation #256

Merged

Conversation

oliverholworthy
Copy link
Member

Remove warning about compound tags deprecation. Since the warning message here is not currently easily actionable by users. And prints a message every time a ColumnSchema is created, resulting in distracting messages in logs.

After discussions about this we are considering keeping the compound tags for backwards compatibility for now.

If we reach a point where we're using the narrower tags only across Merlin, we can revisit the deprecation approach for these

@oliverholworthy oliverholworthy added the chore Maintenance for the repository label Mar 28, 2023
@oliverholworthy oliverholworthy added this to the Merlin 23.03 milestone Mar 28, 2023
@oliverholworthy oliverholworthy self-assigned this Mar 28, 2023
@karlhigley
Copy link
Contributor

Wait, I thought the result of the discussion involved providing a way to select by all tags and using that instead of the compound tags? I'm in favor of the warning going away, but I didn't want to remove it until we actually addressed the underlying issue.

@oliverholworthy
Copy link
Member Author

Providing a way to simplify selecting by all tags was one of the outcomes. However we also discussed two other things. Keeping the compound tags around as a convenient shortcut and removing the warning.

The current state of things seems to be that the warning is not actionable. Which suggests that it would likely be clearer to users of our packages if it wasn't printed/logged.

@karlhigley
Copy link
Contributor

karlhigley commented Mar 31, 2023

The current state of things seems to be that the warning is not actionable.

I think it is actionable now that select_by_tag allows you to choose between union/intersection modes, but 🤷🏻

@karlhigley karlhigley modified the milestones: Merlin 23.03, Merlin 23.04 Apr 4, 2023
@karlhigley karlhigley modified the milestones: Merlin 23.04, Merlin 23.05 Apr 25, 2023
@karlhigley karlhigley merged commit 04ccc31 into NVIDIA-Merlin:main May 4, 2023
@github-actions
Copy link

github-actions bot commented May 4, 2023

Documentation preview

https://nvidia-merlin.github.io/core/review/pr-256

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Maintenance for the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants