Skip to content

Fixed bug in tfx #883

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.

Sign up for GitHub

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

Jump to bottom
Merged
schustmi merged 5 commits into from
Sep 8, 2022
Merged

Fixed bug in tfx #883

schustmi merged 5 commits into from
Sep 8, 2022

Conversation

@htahir1
Copy link
Contributor

@htahir1 htahir1 commented Sep 7, 2022

Describe changes

I implemented a basic patch of TFX to disallow whats mentioned in the comments of the patch

Pre-requisites

Please ensure you have done the following:

  • I have read the CONTRIBUTING.md document.
  • If my change requires a change to docs, I have updated the documentation accordingly.
  • If I have added an integration, I have updated the integrations table and the corresponding website section.
  • I have added tests to cover my changes.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (add details above)

@htahir1 htahir1 requested review from schustmi and strickvl September 7, 2022 13:00
@github-actions github-actions bot added internal To filter out internal PRs and issues bug Something isn't working labels Sep 7, 2022
strickvl
strickvl approved these changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@strickvl strickvl left a comment

Choose a reason for hiding this comment

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

Looks good to me. Made one tiny suggestion, but feel free to ignore.

schustmi
schustmi approved these changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@schustmi schustmi left a comment

Choose a reason for hiding this comment

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

Code looks good, did it fix the issue?

@htahir1
Copy link
Contributor Author

htahir1 commented Sep 8, 2022

yup, now it fails 'nicely' (see screenshot)

image

@schustmi schustmi merged commit 88e79ed into develop Sep 8, 2022
@schustmi schustmi deleted the bugfix/patch_removed_output_dirs branch September 8, 2022 09:50
stefannica pushed a commit that referenced this pull request Sep 14, 2022
@htahir1 @strickvl
@schustmi @stefannica
Fixed bug in tfx (#883)
58e5936 
* Fixed bug in tfx

* Added dict type

* linting changes

* Fix import

* Fix linting

Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com>
Co-authored-by: Michael Schuster <michael.schuster.ffb@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@strickvl strickvl strickvl approved these changes

@schustmi schustmi schustmi approved these changes

Assignees

No one assigned

Labels

bug Something isn't working internal To filter out internal PRs and issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@htahir1 @strickvl @schustmi