byuccl / bfasst

Tools for FPGA Assurance Flows
Apache License 2.0
12 stars 5 forks source link

Flow names on command-line #362

Closed jgoeders closed 10 months ago

jgoeders commented 10 months ago

I noticed that when running flows from the command-line the code converts the class name from PascalCase to snake_case. Is this done to keep lower-case command-line convention? In my opinion it might be more clear to just have the command-line name match the class name, even if it means we are case-insensitive. Thoughts?

jgoeders commented 10 months ago

consensus: case-sensitive name matching the class

KeenanRileyFaulkner commented 10 months ago

The one problem with this is how we obtain access to each flow class at the moment. We use the following utility method in the flow manager, which has to import the module before it gets the flow class.

def get_flow(flow_name):
    """Get a flow by name"""
    flow_module = import_module(f"bfasst.ninja_flows.{flow_name}")
    flow_class = "".join(word.capitalize() for word in flow_name.split("_"))
    return getattr(flow_module, flow_class)

If we were to switch to specifying the name of the class instead of the name of the module, we would have to import all the flow modules and search each one until we find the one that contains the class we specified. Is that what we want to do?

jgoeders commented 10 months ago

Oh I didn't realize it was based on module name.

How are we loading the list for the argparse choices?

KeenanRileyFaulkner commented 10 months ago

It appears that choices was changed to an error handling action when bfasster.py moved to run.py. The flows were - and still are - enumerated with this other flow utility function, which returns the names of all the flow modules' file stems.

jgoeders commented 10 months ago

I think it would be ok to have a single file (perhaps yaml) where we keep a list of flows and their description, which would be used for #382.

KeenanRileyFaulkner commented 10 months ago

Ok, but we'd keep the same approach for accessing them, right? Or we want to change to import all the modules and find the correct flow class?

jgoeders commented 10 months ago

I'd like to avoid importing everything. The yaml could have the info we need to initiate the import. eg:

flows:
- name: vivado
  description: Runs Xilinx Vivado from RTL to bitstream
  module: vivado
  class: VivadoFlow

- name: vivado_ooc
  description: Runs Xilinx Vivado from RTL to implementation, with out-of-context synthesis
  module: vivado
  class: VivadoOocFlow