Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

CI Check against tensorflow include guard #204

Merged
merged 3 commits into from
Jul 18, 2023
Merged

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented Jul 17, 2023

Before submitting a pull request (PR), please read the contributing guide.

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

Tests the include-guard that denies use of the tools when tensorflow is not available.

What does this PR do?

Adds a short check to make sure that the tensorflow include guard is behaving itself on ubuntu and macOS.

  • pip install -e . should successfully fetch tensorflow and allow import.
  • If tensorflow is not installed, importing the module should fail. Conditions for this test are achieved by having pip uninstall tensorflow.

References

How has this PR been tested?

Locally running the commands produces the outcomes that are checked against.
New workflow has been introduced to check the results are as expected on CI.

Is this a breaking change?

No

Does this PR require an update to the documentation?

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality (unit & integration)
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@willGraham01 willGraham01 changed the title Smoke test for include guard CI Check against tensorflow include guard Jul 17, 2023
@willGraham01
Copy link
Collaborator Author

cellfinder CI checks still fail as per brainglobe/cellfinder#276, but this PR is independent of what's causing that.

@willGraham01 willGraham01 requested a review from a team July 17, 2023 09:31
@willGraham01 willGraham01 marked this pull request as ready for review July 17, 2023 09:31
Copy link
Member

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

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

While this all looks good, we should note that the original issue this closes was only encountered on Silicon Macs, and therefore this test wouldn't have caught it (

I think (thought?) we had an issue to expand CI to new Macs, but I can't find it for the life of me. Maybe we should open another one?

@adamltyson
Copy link
Member

I think (thought?) we had an issue to expand CI to new Macs, but I can't find it for the life of me. Maybe we should open another one?

#141

@alessandrofelder alessandrofelder merged commit 0dc6878 into main Jul 18, 2023
@alessandrofelder alessandrofelder deleted the include-guard-test branch July 18, 2023 08:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add test to validate include guards
3 participants