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

VivadoAccelerator backend updates #508

Merged
merged 3 commits into from Mar 28, 2022
Merged

VivadoAccelerator backend updates #508

merged 3 commits into from Mar 28, 2022

Conversation

thesps
Copy link
Contributor

@thesps thesps commented Mar 15, 2022

A few small things:

  • Fix registration of VivadoAcceleratorBackend writer flow. In master branch, the writer flow for that backend is just VivadoBackend's writer flow relabelled, so the extra files for VivadoAcceleratorBackend aren't written out.
  • Replace VivadoAcceleratorBackend.make_bitfile method with build(bitfile=True). This is an API change to make that workflow more consistent with the usual build API (rather than requiring one call to build then one call to make_bitfile)
  • Add a package method to make a .tar.gz archive with all the necesary files for hardware inference - otherwise the user needs to do some finding and renaming of files themselves

with tarfile.open(output_filename, "w:gz") as tar:
tar.add(source_dir, arcname=os.path.basename(source_dir))

def _copy_wait_retry(src, dst, sleep=60):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why this hack is necessary? Or, in other words, why isn't the build process blocking so that the package can start only when we're sure the files are there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I observed cases where the call to os.system returns before the file is written. I guess the process that writes the bitfile is detached.

Copy link
Contributor

Choose a reason for hiding this comment

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

What guarantee do you have the the file is fully written when you try to copy it? Perhaps instead of os.system we run subprocess.Popen() because the object it returns has a wait(). Not sure if will help if the main vivado process calls another one and exits.

@thesps
Copy link
Contributor Author

thesps commented Mar 17, 2022

I got rid of the package function for now. I can't replicate the bitfile writing issue for the moment, it seems to depend on targeting the larger FPGA which takes a long time to build and so getting to the bottom of it will take longer. I intend to add it back as a follow up.

So now the PR does:

  • Fix registration of VivadoAcceleratorBackend writer flow. In master branch, the writer flow for that backend is just VivadoBackend's writer flow relabelled, so the extra files for VivadoAcceleratorBackend aren't written out.
  • Replace VivadoAcceleratorBackend.make_bitfile method with build(bitfile=True). This is an API change to make that workflow more consistent with the usual build API (rather than requiring one call to build then one call to make_bitfile)

@vloncar vloncar merged commit eee9ec8 into master Mar 28, 2022
@jmduarte jmduarte deleted the va-write-fix branch November 2, 2022 02:35
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.

None yet

3 participants