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

Issue 41 cimulator buid #47

Open
wants to merge 73 commits into
base: bump-build-version
Choose a base branch
from

Conversation

alex124585
Copy link
Contributor

Add test which check that c simulator is built correctly

Copy link
Contributor

@ZichangHe ZichangHe 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!

@rsln-s rsln-s self-requested a review March 29, 2024 13:51
Copy link
Contributor

@rsln-s rsln-s left a comment

Choose a reason for hiding this comment

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

A few minor comments. Please also note that the workflow should succeed in Github actions (currently failing).

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated
super().run
except Exception as e:
print("No C/C++ enviroment setup to compile the C simulator. Installing Python Simulator")
if python_only is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this condition be used directly on line 23 below instead of creating an intermediary QOKIT_PYTHON_ONLY variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need that env variable to control workflow/debug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to if python_only is not None:

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like more tests are failing now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like more tests are failing now...

It should right? This is python build only no c-build

Copy link
Contributor

Choose a reason for hiding this comment

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

Why did the tests start failing though? Responding in this thread, but not sure which change triggered it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why did the tests start failing though? Responding in this thread, but not sure which change triggered it

E AssertionError: assert 'c' in ['python']
E + where ['python'] = get_available_simulator_names('x')
Trigged by env variable setup QOKIT_PYTHON_ONLY=True. Intended behavior for that test.

Why did the tests start failing though? Responding in this thread, but not sure which change triggered it

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's intended, it should catch the error, no?

tests/test_simulators_builds.py Outdated Show resolved Hide resolved
@rsln-s rsln-s self-requested a review April 26, 2024 15:04
from qokit.fur import get_available_simulator_names

# when fail tests only runs in Github actions. Change to true to run locally
IN_GITHUB_ACTIONS = os.getenv("GITHUB_ACTIONS") == "fales"
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo! "fales" -> "fails"

IN_GITHUB_ACTIONS = os.getenv("GITHUB_ACTIONS") == "fales"


@pytest.mark.skipif(IN_GITHUB_ACTIONS, reason="Test runs only in Github Actions.")
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: add graceful handling of this in python pipeline

@rsln-s
Copy link
Contributor

rsln-s commented Aug 2, 2024

Status update: there are two main problems
(i) the PYTHON_ONLY flag is not being raised properly in "python only" workflow:
https://github.com/jpmorganchase/QOKit/actions/runs/10218741603/job/28275531066?pr=47
(ii) the simulator is not being built properly on MacOS:
https://github.com/jpmorganchase/QOKit/actions/runs/10218741601/job/28275532498?pr=47

@rsln-s
Copy link
Contributor

rsln-s commented Aug 2, 2024

Update: the Mac build failure is due to clang not supporting openmp. Installing GCC should resolve the issue, but I can't get make to use the right compiler

@rsln-s rsln-s marked this pull request as draft August 5, 2024 22:48
@alex124585 alex124585 marked this pull request as ready for review September 12, 2024 20:50
@alex124585
Copy link
Contributor Author

fixed following pipeline issues:

  1. Mac OS was added with gcc brew install but in Mac OS default path is to Apple clang
  2. Added correct path for gcc - export PATH=$PATH:/opt/homebrew/
  3. Corrected test case for python only
  4. Removed incorrectly added make in pipeline. No need for make since make is in setup.py

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.

4 participants